Please review: 8006498 and 8008474

[Mikael Vidstedt]

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? I can do that as part of the pending 
-Wunused changes if you don't want to update it now.

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
>>>>>>>>
>>>>>>>
>>>>
>>
>