On Thu, 15 Feb 2024 14:13:19 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
> This is a follow-up to > [JDK-8325877](https://bugs.openjdk.org/browse/JDK-8325877). I was careful in > that bug not to change order of any lines, only split up the original file > into parts. > > In this PR, I use the new structure to improve the design. I collect the > functionality into three clearly separate phases, preparation, compilation > and linking. I use consistent naming to reveal whether a function just sets > up variables, or create output. I simplify and split up a few extra hard to > read functions. I move some code around so it is placed more logically. > > It can be hard to convince yourself that the changes are correct if you just > look at the end result. I have tried to make small and clear changes in each > separate commit, and to explain in the commit title what I am doing. I > recommend reviewing this PR [commit by > commit](https://github.com/openjdk/jdk/pull/17873/commits). Overall this looks good. make/common/NativeCompilation.gmk line 132: > 130: SetupNativeCompilation = $(NamedParamsMacroTemplate) > 131: define SetupNativeCompilationBody > 132: # In this functions, helpers named Setup<Foo> are just setting > variables. Suggestion: # In this function, helpers named Setup<Foo> are just setting variables. I think have tried to use the word "macro" rather than "function" in makefiles as that's what they technically are. make/common/native/Paths.gmk line 195: > 193: > 194: > ################################################################################ > 195: define RemoveSuperfluousOutputFiles This macro doesn't fit in either of the two categories mentioned at the top. Perhaps worth clarifying that it's different here. It performs direct actions on output files but also doesn't do it through a recipe. ------------- PR Review: https://git.openjdk.org/jdk/pull/17873#pullrequestreview-1891021386 PR Review Comment: https://git.openjdk.org/jdk/pull/17873#discussion_r1496197461 PR Review Comment: https://git.openjdk.org/jdk/pull/17873#discussion_r1496208627
