RFR(M): 8233787: Break cycle in vm_version* includes

Schmidt, Lutz lutz.schmidt at sap.com
Fri Nov 15 12:19:29 UTC 2019


Hi Kim, 

thanks for reviewing - I understand your comments that way. One more review to go. :-)

I made abstract_vm_version.cpp #include vm_version.hpp, and I updated the copyrights. See the webrev#03:
  http://cr.openjdk.java.net/~lucy/webrevs/8233787.03/ 

I ran the initial webrev iteration through dev-submit and had it active SAP-internally. The current webrev is active since last night SAP-internally. All builds are green. The test show only unrelated issues (some JIT compiler asserts). Of course I will run the final webrev through dev-submit.

Re test coverage: we do not cover 32-bit platforms. And we do not have zero or minimal builds.

Thanks,
Lutz


On 15.11.19, 07:17, "Kim Barrett" <kim.barrett at oracle.com> wrote:

    > On Nov 14, 2019, at 11:26 AM, Schmidt, Lutz <lutz.schmidt at sap.com> 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.
    
    Ick, that's unfortunate.  Almost makes me regret suggesting the new
    file.  Oh well.  I was going to suggest you should get a review from a
    build expert for this, but the changes are quite mechanical and
    obvious.
    
    > Other than that, I hope the new webrev is even closer to your dreams:
    >   http://cr.openjdk.java.net/~lucy/webrevs/8233787.02/ 
    
    I think abstract_vm_version.cpp should #include vm_version.hpp.  That
    way, if Abstract_VM_Version provides any shared helper functions that
    are defined in terms VM_Version values, it can get any potentially
    overridden values.
    
    There currently isn't anything like that, though there could be (and
    perhaps should be, though currently presumably doesn't matter).  See
    jvm_version(), and the initializers for _s_vm_release and
    _s_internal_vm_info_string.  I think you shouldn't do anyhing about
    these references in this change.
    
    There are also a bunch of files with out of date copyrights.  I should
    have mentioned that before.
    
    Other than that, this looks good.
    
    I assume you are running this through dev-submit and SAPs build farm
    to check various platforms.  If there are any not covered by that, you
    should reach out to appropriate platform maintainers.
    
    



More information about the hotspot-dev mailing list