RFR (XXS): 8208672: Enable -Wreorder in make files

[David Holmes]

On 9/08/2018 4:53 AM, Magnus Ihse Bursie wrote:
> Quick reply from my vacation... there's a macro that tests if a compiler option is valid. We've only used it for gcc so far, but I see no reason for why it should not work with VS. Maybe you can use that instead of checking version?

Thanks for the response Magnus. I think your suggestion will involve 
being able to locally verify with different versions of VS to ensure the 
macro test actually works as expected. And neither Thomas nor I are able 
to do that, so ...

> I'm a bit hesitant at adding options that will produce warnings for older but still supported versions. If it's just a single compiler at the end of the list, that's one thing, but if we're talking everything but VS2017, that's another. If there's a problem adding the VS option, I'd rather see you just add the gcc one.

... looks like we're just skipping Windows.

Thanks,
David
> 
> /Magnus
> 
>> 7 aug. 2018 kl. 12:08 skrev David Holmes <david.holmes at oracle.com>:
>>
>> Hi Thomas,
>>
>>> On 7/08/2018 7:34 PM, Thomas Schatzl wrote:
>>> Hi David,
>>>> On Tue, 2018-08-07 at 07:17 +1000, David Holmes wrote:
>>>> Hi Thomas,
>>>>
>>>>> On 6/08/2018 10:38 PM, Thomas Schatzl wrote:
>>>>> Hi David,
>>>>>
>>>>>> On Fri, 2018-08-03 at 10:20 +1000, David Holmes wrote:
>>>>>> Hi Thomas,
>>>>>>
>>>>>>> On 2/08/2018 7:14 PM, Thomas Schatzl wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>>      there have been several suggestions to try to fix the
>>>>>>> Hotspot code to allow us to enable -Wreorder in the Hotspot
>>>>>>> sources.
>>>>>>> This should make problems due to use-before-initialization much
>>>>>>> more obvious.
>>>>>>>
>>>>>>> This change enables -Wreorder for gcc and clang.
>>>>>>
>>>>>> For Windows (VS2017+) see:
>>>>>>
>>>>>> https://docs.microsoft.com/en-us/cpp/error-messages/compiler-
>>>>>> warnings /c5038
>>>>>>
>>>>>> Otherwise change seems okay.
>>>>>>
>>>>>
>>>>> Thanks!
>>>>>
>>>>> http://cr.openjdk.java.net/~tschatzl/8208672/webrev.0_to_1 (diff)
>>>>> http://cr.openjdk.java.net/~tschatzl/8208672/webrev.1 (full)
>>>>>
>>>>> I verified that all our platforms (including Windows) still build.
>>>>
>>>> I think the Windows change needs to be based on the compiler version
>>>> used as, from what I read, the flag only exists in VS2017.
>>>    jdk11 official compiler is VS2017 so I figured it would not be that
>>> much of a problem.
>>
>> Right now the build supports a range of compilers for the community:
>>
>> ################################################################################
>> # The order of these defines the priority by which we try to find them.
>> VALID_VS_VERSIONS="2017 2013 2015 2012 2010"
>>
>>> Another reason for my thinking is that MSVC 2013 does not implement
>>> some interesting C++11 features anyway so we might be forced to drop
>>> support for it soon (and the situation is of course worse if we upgrade
>>> to C++14).
>>
>> If that happens we'd have to deal with it sure ...
>>
>>> I looked a bit at the makefile and conditionalizing this on VS2017 (or
>>> checking only whether VS2013 let it slide, i.e. give an "unsupported
>>> option" warning but continue anyway because I would need to setup a
>>> Windows dev environment somewhere) would take me a lot of time.
>>
>> Hoping the build folk will jump in as this should be pretty straight-forward I think - just a question of knowing what VERSION variable to test.
>>
>>> Would it be possible to skip -Werror support for Visual Studio now and
>>> try to fix this in a later enhancement?
>>
>> Sure. Just a pity as it will be 10x as much work to add later by itself.
>>
>> Thanks,
>> David
>> -----
>>
>>> Thanks,
>>>    Thomas
>