Hello Matthias,
While your change is ok and can certainly be pushed on its own, there is
so much more needing cleanup here. If you don't mind, here is what I
think needs doing:
* Inline BUILD_JSPAWNHELPER_SRC, JSPAWNHELPER_CFLAGS,
BUILD_JSPAWNHELPER_DST_DIR and LINK_JSPAWNHELPER_OBJECTS. Since these
variables aren't conditionally changed anywhere, there is really no need
for the indirection.
* The whole business of "BUILD_JSPAWNHELPER_LDFLAGS +=
$(COMPILER_TARGET_BITS_FLAG)64" is confusing to me. Don't we trust the
compiler for a 64 bit target to produce a 64 bit binary given the
standard CFLAGS_JDKEXE and LDFLAGS_JDKLIB? I suspect this is just very
old and confused code
* The src dir only has the one src file, no need to explicitly list it
for include.
* The adding of childproc.o to LIBS can be accomplished using the
parameter EXTRA_OBJECT_FILES. By using that you automatically get the
dependency declaration so you can remove the line
"$(BUILD_JSPAWNHELPER): $(LINK_JSPAWNHELPER_OBJECTS)"
* The ifeq ($(BUILD_JSPAWNHELPER), 1) is also annoying, just move the
single conditional into it's place.
Thanks
/Erik
On 2017-02-02 17:25, Baesken, Matthias wrote:
Hello,
could I please have a review for the following small change that
cleans up the special macosx BUILD_JSPAWNHELPER_DST_DIR setting that is not
really
needed any more after CR 8066474: "Remove the lib/ directory from Linux and
Solaris images" changed the default setting .
Bug :
https://bugs.openjdk.java.net/browse/JDK-8173834
webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8173834.0/
Thanks, Matthias
