On Mon, 4 Dec 2023 11:39:04 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reflecting review feedback - keeping CT_TRANSITIVE_MODULES separate from >> CT_MODULES. > > make/modules/jdk.compiler/Gendata.gmk line 41: > >> 39: CT_MODULES := $(filter-out $(MODULES_FILTER), $(DOCS_MODULES)) >> 40: CT_EXTRA_MODULES := jdk.unsupported >> 41: CT_TRANSITIVE_MODULES := $(call FindTransitiveIndirectDepsForModules, >> $(CT_MODULES)) $(CT_MODULES) > > Now the CT_TRANSITIVE_MODULES will include all CT_MODULES. I am pretty sure > the intention was to keep them separated, for clarity, but to act on both > CT_MODULES and CT_TRANSITIVE_MODULES. See e.g. the foreach on line 43. (This > change will also make that foreach process CT_MODULES twice.) > > Also, just for code readability, can you move the hard-coded list of extra > modules to after the programmatic definitions of CT_MODULES and transitive > modules, perhaps even with an empty line in between? > > I think a better solution is to change your new line to: > > $(ECHO) $(CT_MODULES) $(CT_TRANSITIVE_MODULES) $(CT_EXTRA_MODULES) > >$(SUPPORT_OUTPUTDIR)/symbols/included-modules Thanks for the comment! I tried to act on it here: https://github.com/openjdk/jdk/pull/16942/commits/879277891708533b0b56dbe36b20760c62708bbf ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16942#discussion_r1414515956
