Hi Erik, Thanks for the review!
> On 10 Sep 2018, at 19:09, Erik Joelsson <erik.joels...@oracle.com> wrote: > > Hello Robin, > > Overall very nice work. A few things though: > > The output dir compile_commands should use a dash instead to fit with current > patterns. That way the clean target will also be all dashes. Fixed. I actually removed the clean target, as the output is located in a subfolder of make-support, so probably don’t need a special one. > Main.gmk:888: I think you meant to add test-compile-commands here, which also > requires you to define that target. Fixed, the tests are now invoked from a top-level target. I also added a second test to ensure that the no-build-libraries property of this actually stays intact. > In NativeCompilation.gmk:739: I don't see a reason for = assignment, so > please use :=. On the next line, please indent for continuation with 4 spaces. Fixed. > In Launcher-jdk.pack.gmk, this SetupJdkExecutable can be rewritten to use the > EXTRA_OBJECT_FILES parameter (inherited from SetupNativeCompilation macro) > for UNPACKEXE_ZIPOBJS instead of adding them to LDFLAGS. Then the explicit > dependency declaration can go away completely. Fixed. > In Awt2dLibraries.gmk, I think lines 733 and 735 needs to be fixed as well. > Using FindLib is generally a good idea for this. I suspect there may be more > such instances sprinkled around the makefiles. Only fixed the last one, I think the first one is ok as is? Webrev (incremental): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00-01/ Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01/ Testing: tier1, builds-tier5 Best regards, Robin > > /Erik > > > On 2018-09-10 06:36, Robin Westberg wrote: >> Hi all, >> >> Please review the following change that adds support for generating >> compile_commands.json as a top-level make target. This is a popular format >> for describing how to compile object files for a project, and is defined in >> [1]. This file can be used directly by IDEs such as Visual Studio Code and >> CLion as well as "language servers" such as cquery [2] and rtags [3]. >> >> While it’s currently possible to generate this file as part of a full build >> (using tools such as Bear [4], or simply parsing .cmdline files), this >> change aims to make it possible to generate the compile commands data >> without actually compiling the object files. This means that it’s for >> example possible to start using an IDE fairly quickly on freshly cloned >> source, instead of having to wait for a full build to complete. >> >> This was originally inspired by the work done in [5] by Mikael Gerdin and >> Erik Joelsson, but has been through a few revisions since then. Erik has >> also provided a lot of assistance with the current version, thanks Erik! >> >> Issue: https://bugs.openjdk.java.net/browse/JDK-8210459 >> Webrev: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00/ >> Testing: tier1, builds-tier5 >> >> Best regards, >> Robin >> >> [1] https://clang.llvm.org/docs/JSONCompilationDatabase.html >> [2] https://github.com/cquery-project/cquery >> [3] https://github.com/Andersbakken/rtags >> [4] https://github.com/rizsotto/Bear >> [5] http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-March/026354.html >
