RFR: 8290373: Enable lossy conversion warnings on Windows [v4]

[Jorn Vernee]

On Wed, 20 Jul 2022 14:18:49 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> This patch enables lossy conversion warnings (C4244 [1]) for hotspot on Windows/MSVC. Instead of fixing all warnings that were produced from this, I've instead locally disabled the warning in the files that produced warnings. This allows gradually making progress with cleaning up these warnings on a per-file basis, instead of trying to fix all of them in one shot. i.e. it is not meant as a long term solution, but as a way of allowing incremental progress.
>> 
>> Out of the ~1100 files that make up hotspot on Windows x64 , ~290 have warnings for them disabled (not counting aarch64 files), which means that with this patch ~800 files are protected by enabling this warning globally.
>> 
>> Warnings can be fixed in individual files, or groups of files in followup patches, and warnings for those files can be enabled.
>> 
>> I'm working on a patch that does the same for GCC, but it produces warnings in about 150 more files, so I wanted to gather feedback on this approach before continuing with that.
>> 
>> ---
>> 
>> To disable warnings for a file, in most cases the following prelude is added after the last `#include` at the start of a file:
>> 
>>     PRAGMA_DIAG_PUSH
>>     PRAGMA_ALLOW_LOSSY_CONVERSIONS
>> 
>> And then the following is added at the end of the file for cpp files, or before closing the header guard for hpp files:
>> 
>>     PRAGMA_DIAG_POP
>> 
>> 1 notable exception are files produced by adlc, which had their code-gen modified to add these lines instead. There were also 2 files that include headers in the middle of the file (ostream.cpp & sharedRuntime.cpp), for which I've added the PRAGMA's after the include block at the start of the file instead. They only included system headers, for which disabling warnings doesn't matter any ways.
>> 
>> [1]: https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-levels-3-and-4-c4244?view=msvc-170
>
> Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits:
> 
>  - Merge branch 'master' into Warn_Narrow
>  - Polish pt2
>  - Polish
>  - Remove PUSH POP from test files
>  - Remove PUSH POP from cpp files
>  - Rest of the tests
>  - More test
>  - AArch64
>  - Disable for tests
>  - Fix apostrophe
>  - ... and 5 more: https://git.openjdk.org/jdk/compare/1c055076...fb276afd

I think these warnings are a nice-to-have. Kinda like a certain code style. However, I don't think it's important enough to organize people and go through fixing each and every one of them in the 450 or so files all at once.

The warnings being either unimportant enough to turn them off like we do now, or being important enough to turn them on and fix all of them is, I think, a false dichotomy. In reality, the importance of fixing these warnings seems to be more in the middle. And I think this is what leads to a stalemate (the original JBS issue for this was filed in 2015 [1]). So, I'd like to suggest a compromise instead. 

We could have a policy similar to how styling is fixed currently in HotSpot (mostly in the compiler code). In that case styling is fixed in the surrounding code that is touched by a patch.

In the case of these warnings, I propose that when a patch touches a file with lossy conversion warnings disabled, it should also enable the warnings and fix them for that file. Maybe it's useful to rename the macro to something like `PRAGMA_FIXME_LOSSY_CONVERSIONS` for that as well.

[1] : https://bugs.openjdk.org/browse/JDK-8135181

-------------

PR: https://git.openjdk.org/jdk/pull/9516