On Wed, 20 Jul 2022 14:18:49 GMT, Jorn Vernee <[email protected]> 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 While I normally appreciate enabling warnings, and fixing them, I tend to side a bit with David on this one. There is a lot of `PRAGMA_ALLOW_LOSSY_CONVERSIONS` in this PR, injected into the files, forever to be forgotten. I do agree with @JornVernee that warnings are in a bit of a bind; if we really think a warning is useful, but it has been historically turned off, it is a huge uphill battle to get it enabled, by fixing all historical issues at once. This results in warnings being left turned off, even for new code, since no-one has the time available to address all issues at once. I'm thinking that part of this problem is due to the lack of granularity that the build systems offers to enable/disable warnings. We have a "global" enable, and then we have a "per dll" disable. It would not be too hard for me to modify this, so we could make a list of source files for which the conversion warning still needs to be turned off. If this is a coherent list in a single make file, it is much easier to see what remains to be done to actually fix these issues. (This will not work for .hpp files; these will still need the pragma push/pop thing, and should therefore be prioritized when actually fixing this.) Still, the warnings need to be fixed (or the list will need to be kept indefinitely, which is of course also a valid solution). And we need to be prepared to put in effort to actually resolve these, if we really want this to happen. ------------- PR: https://git.openjdk.org/jdk/pull/9516
