On Thu, 14 Mar 2024 15:16:53 GMT, Magnus Ihse Bursie <[email protected]> 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
