On Thu, 14 Mar 2024 15:16:53 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
>> We are setting one of the flags `CFLAGS_JDKLIB`, `CXXFLAGS_JDKLIB`, >> `CFLAGS_JDKEXE` or `CXXFLAGS_JDKEXE` to `CFLAGS` or `CXXFLAGS`, >> respectively, in basically all calls to `SetupJdkLibrary` and >> `SetupJdkExecutable`. >> >> These flag variables contain a lot of duplication. >> >> The first step towards bringing some sanity to this mess is to move the >> setting of these variables into `SetupJdkLibrary/SetupJdkExecutable`. >> >> In a few places these standard flags are not set, partially or fullly. This >> is handled by the new arguments `DEFAULT_CFLAGS := false` (to disable the >> entire setting) and `C[XX]FLAGS_FILTER_OUT` (which excludes some specific >> flag) to `SetupJdkLibrary/SetupJdkExecutable`. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix syntax error make/common/JdkNativeCompilation.gmk line 206: > 204: $$(foreach flag, $$($1_CXXFLAGS_FILTER_OUT), \ > 205: $$(eval $1_CXXFLAGS := $(filter-out $$(flag), $$($1_CXXFLAGS)) \ > 206: )) There shouldn't be a need for a loop here. Suggestion: $$(filter-out $$($1_CXXFLAGS_FILTER_OUT), $$($1_CXXFLAGS)) Avoiding eval calls is usually a good idea as `$` escaping gets really convoluted when nesting them. Also, the single `$` in the filter-out call can't possibly work? The conditional is also not necessary as filter-out with an empty pattern is a noop, so all of this could be condensed to a single line, including the assignment. Same thing applies to a couple of more places below. make/modules/jdk.jpackage/Lib.gmk line 63: > 61: CXXFLAGS_FILTER_OUT := -MD, \ > 62: CXXFLAGS := $(JPACKAGE_APPLAUNCHER_INCLUDES), \ > 63: CFLAGS := $(JPACKAGE_APPLAUNCHER_INCLUDES), \ This removed `-MD`, but I don't see it adding `-MT`. make/modules/jdk.jpackage/Lib.gmk line 140: > 138: OPTIMIZATION := LOW, \ > 139: SRC := $(JPACKAGE_WIXHELPER_SRC), \ > 140: CXXFLAGS_FILTER_OUT := -MD, \ Same here about `-MT`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18301#discussion_r1525301430 PR Review Comment: https://git.openjdk.org/jdk/pull/18301#discussion_r1525409694 PR Review Comment: https://git.openjdk.org/jdk/pull/18301#discussion_r1525410729