On Mon, 2 Feb 2026 12:45:50 GMT, Leo Korinth <[email protected]> wrote:
> Allow conversion warnings in subsets of the code base. By allowing this, we > can improve the code base in parts, and see that those parts do not regress > in the future. > > My approach to implement this is by adding support to our make system to > recognise and handle "variable packs". A "variable pack" is a list of quoted > variable "appendings". It will be picked up by `NamedParamsMacroTemplate` and > there recognised by the lack of an assignment operator that is always used > when sending variables to macros today. To support sending lists of "variable > appendings", the appendings must quote assignment, spaces and quotes. This > would be cleanest to implement by hex or base64 encode the string. However, > this is extremely hard to do in make, and I prefer not calling the likes of > `od` or `base64` to make the code portable and fast. > > With this infrastructure I implement a simple recursive utility to find all > files matching a pattern in a folder; I then transform that list to variable > assignments that will add compiler warnings for those files. > > This approach is extremely flexible. I can for example combine many calls to > the `overrideFlags` macro with different source directories and different > patterns. > > The macro will expand to something like (depending on compiler): > `module_file1.cpp_CXXFLAGS+=-Wconversion` > `module_file2.cpp_CXXFLAGS+=-Wconversion` > > this can flexibly be combined with other flags to overlap: > `module_file2.cpp_CXXFLAGS+=$(SPACE)-Wotherflag` > `module_file3.cpp_CXXFLAGS+=$(SPACE)-Wotherflag` > > (note the overlapping sets of flags `file1 -Wconversion`, `file2 -Wconversion > -Wotherflag`, `file3 -Wotherflag`) There is some overlap in purpose between the build system changes here and those in my proposed change for JDK-8332189 (https://github.com/openjdk/jdk/pull/29497). While this change is more general purpose, it's not obvious to me that it would actually get applied anywhere other than to HotSpot. Being selective about the scope for adding options has both good and bad points. That applies to both approaches. My feeling is the bad outweighs the good here. I don't think I like the idea of having ghettos within HotSpot where different warning options are applied. We already have some of that with per-file disabling of warning, which we don't seem to be making much progress on reducing, even for HotSpot. I've not made any attempt to seriously review the new infrastructure here. I'd prefer to leave that to people with a better understanding of makefile programming and of our build system. make/autoconf/flags-cflags.m4 line 238: > 236: DISABLED_WARNINGS="$DISABLED_WARNINGS psabi" > 237: fi > 238: CFLAGS_CONVERSION_WARNINGS="-Wconversion" As I said in [JDK-8135181](https://bugs.openjdk.org/browse/JDK-8135181) think gcc's `-Wconversion` does too much, and we should for now restrict to conversions between integer types. So also need `-Wno-float-conversion`. make/autoconf/flags-cflags.m4 line 254: > 252: # false positives. > 253: DISABLED_WARNINGS="unknown-warning-option unused-parameter" > 254: CFLAGS_CONVERSION_WARNINGS="-Wconversion -Wno-sign-conversion" For clang I think we should instead use `-Wimplicit-int-conversion`, again to restrict to conversions between integer types for now. make/common/MakeBase.gmk line 173: > 171: unquoteAppendIndex = $(call unQuote,$(word $2,$(subst > $(appendQ),$(SPACE),$1))) > 172: > 173: # $(call requrseFiles,dir) -> header.hpp file1.cpp file2.cpp What is "requrse"? I suspect a (consistent) misspelling of something? make/hotspot/lib/CompileJvm.gmk line 192: > 190: abstract_vm_version.cpp_CXXFLAGS := $(CFLAGS_VM_VERSION), \ > 191: arguments.cpp_CXXFLAGS := $(CFLAGS_VM_VERSION), \ > 192: $(call > overrideFlags,$(TOPDIR)/src/hotspot/share/gc/g1,g1ConcurrentMark.cpp,_CXXFLAGS,$(CFLAGS_CONVERSION_WARNINGS)), > \ `overrideFlags` seems like the wrong name for this? Isn't it _extending_ the flags? ------------- Changes requested by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/29523#pullrequestreview-3743225824 PR Review Comment: https://git.openjdk.org/jdk/pull/29523#discussion_r2757406925 PR Review Comment: https://git.openjdk.org/jdk/pull/29523#discussion_r2757411919 PR Review Comment: https://git.openjdk.org/jdk/pull/29523#discussion_r2757451590 PR Review Comment: https://git.openjdk.org/jdk/pull/29523#discussion_r2757436874
