Please review: 8006498 and 8008474

[JOSEPH PROVINO]

On 2/28/2013 9:09 PM, Mikael Vidstedt wrote:
>
> Looks good!
>
> Do you think it'd make sense to move "-Wpointer-arith -Wsign-compare 
> -Wundef" out of the ifneq for {bsd,linux}/Makefile/gcc.make and only 
> leave the -Wconversion in there?

yes, that makes sense.

> I can do that as part of the pending -Wunused changes if you don't 
> want to update it now.

That would be great.  Since I have two reviews, I'd just as soon push now.

thanks.

joe

>
> Cheers,
> Mikael
>
> On 2013-02-28 17:56, JOSEPH PROVINO wrote:
>> Latest webrev is here: 
>> http://cr.openjdk.java.net/~jprovino/8006498/webrev.01
>>
>> joe
>>
>>
>> On 2/25/2013 1:33 PM, Mikael Vidstedt wrote:
>>>
>>> On 2/24/2013 2:59 PM, David Holmes wrote:
>>>> On 23/02/2013 12:35 AM, Joseph Provino wrote:
>>>>> On 02/21/2013 10:13 PM, David Holmes wrote:
>>>>>> On 22/02/2013 12:59 PM, David Holmes wrote:
>>>>>>> On 22/02/2013 12:48 AM, JOSEPH PROVINO wrote:
>>>>>>>> Actually, from looking at this again, I thought David was 
>>>>>>>> suggesting
>>>>>>>> that -Wundef
>>>>>>>> should be added to ACCEPTABLE_WARNINGS for linux and bsd.
>>>>>>>
>>>>>>> I was being ambivalent - I just wanted to see consistency. We 
>>>>>>> should see
>>>>>>> where Mikael is adding -Wunused
>>>>>>
>>>>>> Ha! Now this really muddies the waters (this is linux only):
>>>>>>
>>>>>> ! ADDITIONAL_WARNINGS = -Wunused-function
>>>>>> !
>>>>>> ! CFLAGS_WARN/DEFAULT = $(WARNINGS_ARE_ERRORS) 
>>>>>> $(ACCEPTABLE_WARNINGS)
>>>>>> $(ADDITIONAL_WARNINGS)
>>>>>
>>>>> That's the problem -- it's already inconsistent.
>>>>
>>>> The names do not make sense to me but perhaps I am misunderstanding 
>>>> how this works. Are we listing all the warnings that will be 
>>>> errors, or are we listing those warnings that we don't want to be 
>>>> errors? I'm not understanding what acceptable nor additional are 
>>>> supposed to mean here.
>>>
>>> I can't say that I see any real patterns in how these variables are 
>>> used either. My guess is that at some point somebody figured out a 
>>> few warnings that were "acceptable" - as in warnings that we could 
>>> turn on without getting tons of false positives and incorrectly 
>>> generated code, but that's just a guess. Unless somebody has 
>>> insights around why these are named and split up the way they are I 
>>> suggest that we simplify and unify all of it.
>>>
>>> My suggestion is to have all the warning flags go into a variable 
>>> called WARNING_FLAGS, leaving only two variables: 
>>> WARNINGS_ARE_ERRORS and WARNING_FLAGS. Something like this (for 
>>> Linux, similar for Solaris):
>>>
>>> WARNING_FLAGS = -Wpointer-arith -Wsign-compare -Wunused-functions 
>>> -Wundef
>>>
>>> # Since GCC 4.3, -Wconversion has changed its meanings to warn these 
>>> implicit
>>> # conversions which might affect the values. To avoid that, we do 
>>> not use it.
>>> ifeq "$(shell expr \( $(CC_VER_MAJOR) \> 4 \) \| \( \( 
>>> $(CC_VER_MAJOR) = 4 \) \& \( $(CC_VER_MINOR) \>= 3 \) \))" "0"
>>> WARNING_FLAGS += -Wconversion
>>> endif
>>>
>>> CFLAGS_WARN/DEFAULT = $(WARNINGS_ARE_ERRORS) $(WARNING_FLAGS)
>>>
>>> Reasonable?
>>>
>>> Cheers,
>>> Mikael
>>>
>>>
>>>>
>>>> David
>>>>
>>>>> joe
>>>>>
>>>>>>
>>>>>>
>>>>>> David
>>>>>>
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>>> joe
>>>>>>>>
>>>>>>>> On 2/21/2013 8:19 AM, Zhengyu Gu wrote:
>>>>>>>>> Agree with David, -Wundef should go to CFLAGS_WARN/DEFAULT on 
>>>>>>>>> Solaris
>>>>>>>>> if there is not particular reason not to.
>>>>>>>>>
>>>>>>>>> Other than that, look good to me.
>>>>>>>>>
>>>>>>>>> -Zhengyu
>>>>>>>>>
>>>>>>>>> On 2/20/2013 7:20 PM, David Holmes wrote:
>>>>>>>>>> On 21/02/2013 7:47 AM, JOSEPH PROVINO wrote:
>>>>>>>>>>> 8006498:  ASSERT and other symbols used incorrectly with #if 
>>>>>>>>>>> are
>>>>>>>>>>> supposed to be defined or not
>>>>>>>>>>>
>>>>>>>>>>> 8008474:  -Wundef should be added to the build to catch #if
>>>>>>>>>>> references
>>>>>>>>>>> to undefined macros.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Webrev is here:
>>>>>>>>>>> http://cr.openjdk.java.net/~jprovino/8006498/webrev.00/
>>>>>>>>>>
>>>>>>>>>> On bsd and linux you simply added -Wundef directly to the 
>>>>>>>>>> CFLAGS:
>>>>>>>>>>
>>>>>>>>>> +CFLAGS_WARN/DEFAULT = $(WARNINGS_ARE_ERRORS) 
>>>>>>>>>> $(ACCEPTABLE_WARNINGS)
>>>>>>>>>> -Wundef
>>>>>>>>>>
>>>>>>>>>> but on Solaris you added it to ADDITIONAL_WARNINGS:
>>>>>>>>>>
>>>>>>>>>> +ADDITIONAL_WARNINGS = -Wpointer-arith -Wconversion 
>>>>>>>>>> -Wsign-compare
>>>>>>>>>> -Wundef
>>>>>>>>>>
>>>>>>>>>> I guess it is a little confusing as to whether 
>>>>>>>>>> ACCEPTABLE_WARNINGS
>>>>>>>>>> and ADDITIONAL_WARNINGS are meant to be the same thing.
>>>>>>>>>>
>>>>>>>>>> Otherwise all the #if -> #ifdef etc seem okay.
>>>>>>>>>>
>>>>>>>>>> David
>>>>>>>>>>
>>>>>>>>>>> Thanks.
>>>>>>>>>>>
>>>>>>>>>>> joe
>>>>>>>>>
>>>>>>>>
>>>>>
>>>
>>
>