Please review: 8006498 and 8008474

[JOSEPH PROVINO]

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