Adding build-dev@... since this changeset now touches make/hotspot/lib/CompileJvm.gmk. The Build team has a standing request to be included on reviews that touch makefiles...
Dan On 11/14/19 11:26 AM, Schmidt, Lutz wrote:
Hi Kim, that wasn't straightforward. Had to adapt make/hotspot/lib/CompileJvm.gmk. Build settings like HOTSPOT_VERSION_STRING have to flow into the compile step of abstract_vm_version.cpp now. For the details, see my comments below. Other than that, I hope the new webrev is even closer to your dreams: http://cr.openjdk.java.net/~lucy/webrevs/8233787.02/ Thanks, Lutz On 14.11.19, 00:34, "Kim Barrett" <[email protected]> wrote: > On Nov 13, 2019, at 11:42 AM, Schmidt, Lutz <[email protected]> wrote: > > Hi Kim, > > there is a new webrev at http://cr.openjdk.java.net/~lucy/webrevs/8233787.01/ > > It should be pretty close to what you view as the "right approach". There weren't too many changes relative to 8233787.00. Most files already had #include runtime/vm_version.hpp.This looks much better to me, but many (most?) of the changed#includes need to be moved into sort order. R: tried my best to fix the sort order. Sorry for not paying attention in the first place.------------------------------------------------------------------------------src/hotspot/share/runtime/vm_version.cppAbstract_VM_Version definitions should be moved to abstract_vm_version.cpp.Maybe just rename the file; I think the only thing that would be left for vm_version.cpp would be VM_Version_init(). But maybe that should be left behind in vm_version.cpp? Though that makes the review messier. R: Everything moved as you suggested. Doesn't make sense to have Abstract_VM_Version:: methods in vm_version.cpp file.------------------------------------------------------------------------------src/hotspot/share/runtime/abstract_vm_version.hppShould #include globalDefinitions.hpp.- uint64_t features() - #define SUPPORTS_NATIVE_CX8 R: Done.Should forward-declare class outsputStream.- print_virtualization_info - print_matching_lines_from_file (I wonder why this is *here*, but not your problem) R: Done.------------------------------------------------------------------------------
