On Wed, 28 Sep 2022 19:52:27 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
>> After [JDK-8294281](https://bugs.openjdk.org/browse/JDK-8294281), it is now >> possible to disable warnings for individual files instead for whole >> libraries. I used this opportunity to go through all disabled warnings in >> hotspot. >> >> Any warnings that were only triggered in a few files were removed from >> hotspot as a whole, and changed to be only disabled for those files. >> >> Some warnings didn't trigger in any file anymore, and could just be removed. >> >> Overall, this reduced the number of disabled warnings by roughly half for >> gcc, clang and visual studio. The remaining warnings are sorted in >> "frequency", that is, the first listed warnings are triggered in the most >> number of files, while the last in the fewest number of files. So if anyone >> were to try to address the remaining warnings, it would make sense to chop >> of this list from the back. >> >> I believe the warnings that are disabled on a per-file basis can most likely >> be fixed relatively easily. >> >> I have verified this by Oracle's internal CI system, and GitHub Actions. >> (But I have not yet gotten a fully green run due to instabilities in GHA, >> however this patch can't reasonably have anything to do with that.) As >> always, warnings tend to differ a bit between compilers, so if someone wants >> to take this on a spin with some other version, please go ahead. If I missed >> some warning, in worst case we'll just have to add it back again, and in the >> meanwhile `configure --disable-warnings-as-errors` is an okay workaround. >> >> It also turned out that JDK-8294281 did not save the new per-file warnings >> in VarDeps, so I had to move $1_WARNINGS_FLAGS from $1_BASE_CFLAGS to >> $1_CFLAGS (and similar for C++). >> >> Annoyingly, the assert macro triggers >> `tautological-constant-out-of-range-compare` on clang, so while this is a >> single problem in a single file, this erupts all over the place in debug >> builds. If this can be fixed, the ugly extra `DISABLED_WARNINGS_clang += >> tautological-constant-out-of-range-compare` for non-release builds can be >> removed. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Revert jvm variants hack make/hotspot/lib/CompileJvm.gmk line 101: > 99: DISABLED_WARNINGS_xlc := tautological-compare shift-negative-value > 100: > 101: DISABLED_WARNINGS_microsoft := 4624 4244 4291 4146 I went through the list of Visual Studio warnings being removed from the global suppression list. clang: tautological-compare windows: 4127 - conditional expression is constant I'm not sure we want these turned on. This is the kind of thing that can happen quite easily with macros and/or in a code base that uses things like conditional compilation to support multiple platforms. Working around these warnings when they arise can make for significant code uglification. OTOH, I guess it *could* catch some programming mistakes. Maybe these could remain surppressed for this change, and discussed separately? 4100 - 'identifier': unreferenced formal parameter I'm very surprised removing this doesn't cause problems, as I'm sure I've seen named but unused parameters. Maybe they've been cleaned up over time? I think some compiler (probably Visual Studio) used to be unhappy with unnamed parameters, which made this hard to avoid in some cases. Fortunately, that doesn't seem to be a problem anymore. Removal okay. 4201 - nonstandard extension used: nameless struct/union Removal okay. 4351 - new behavior: elements of array ‘array’ will be default initialized (according to google, but it is undocumented!!!) This one seems to have been dropped by more recent versions of VS. There are places where we use the feature, expecting the new behavior. Removal okay. 4511 - 'class': copy constructor was implicitly defined as deleted 4512 - 'class': assignment operator was implicitly defined as deleted I think that if the indicated situations were to exist and the warnings hadn't been suppressed that we'd get link-time errors instead. Removal okay. 4514 - 'function': unreferenced inline function has been removed I think we don't want this turned on, but it's also off by default. Removal okay. ------------- PR: https://git.openjdk.org/jdk/pull/10414
