On Tue, 10 Feb 2026 10:03:08 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`) > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > add comment regarding usage of function 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 > extendingFlags,$(TOPDIR)/src/hotspot/share/gc/g1,g1ConcurrentMark.cpp,_CXXFLAGS,$(CFLAGS_CONVERSION_WARNINGS)), > \ Looking at this from a style perspective, we generally name macros with initial letter capitalized. We also tend to use imperative mood, e.g. ExtendFlags rather than extendingFlags. Looking at this call, can you make it work with whitespace after comma, i.e. add calls to `strip` for every parameter in the top level macro. I know it makes the macro definition harder to read, but it's required to be able to line break this call, which it very much needs to be possible to do. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29523#discussion_r2795039465
