Please review: 8006498 and 8008474

[Mikael Vidstedt]

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