On Thu, Mar 28, 2019 at 3:41 PM Andrew Dinn <ad...@redhat.com> wrote:
> On 28/03/2019 12:17, Thomas Stüfe wrote:
> > this looks fine, nits only:
>
> Thank you for the review, Thomas. I'll post a follow up webrev to
> address the comments. Responses are inline.
>
> > UnsafeConstantsFixup::do_field()
> >
> > you could shrink that coding a bit by factoring out the checks, e.g.:
> > . . .
>
> Yes, that was egregious cut-and-paste coding.
>
> Actually, the idea is that all fields of the class are final static and
> all of them are expected to be injected (which is why the final else
> clause asserts).
>
> So, I could just move the asserts up to precede the if-elseif cascade
> and also include checks that the field is final and static.
>
> void do_field(fieldDescriptor* fd) {
> oop mirror = fd->field_holder()->java_mirror();
> assert(mirror != NULL, "UnsafeConstants must have mirror already");
> assert(fd->field_holder() ==
> SystemDictionary::UnsafeConstants_klass(), "Should be UnsafeConstants");
> assert(fd->is_final(), "fields of UnsafeConstants must be final");
> assert(fd->is_static() "fields of UnsafeConstants must be static");
> if (fd->name() == vmSymbols::address_size_name()) {
> mirror->int_field_put(fd->offset(), _address_size);
> } else if (fd->name() == vmSymbols::page_size_name()) {
> mirror->int_field_put(fd->offset(), _page_size);
> } else if (fd->name() == vmSymbols::big_endian_name()) {
> . . .
> } else {
> assert(false, "unexpected UnsafeConstants field");
> }
>
> > Also, is there a reason that code from thread.cpp:
> >
> > jint address_size = sizeof(void*);
> > jint page_size = os::vm_page_size();
> > jboolean is_big_endian = LITTLE_ENDIAN_ONLY(false)
> > BIG_ENDIAN_ONLY(true);
> > jint use_unaligned_access = UseUnalignedAccesses;
> >
> > could not just happen right in UnsafeConstantsFixup::do_field()? You
> > would not have to pass all these arguments around, param list could be
> > empty.
>
> No good reason. Your suggestion is better.
>
> > src/hotspot/share/classfile/vmSymbols.hpp
> >
> > Alignment of trailing '\' broken
>
> Oops!
>
> > src/java.base/share/classes/jdk/internal/misc/UnsafeConstants.java
> >
> > s/intitialize/initialize
> > s/iff/if
> The second of those was actually intended to be iff. This is a common
> abbreviation used by English/US mathematicians and logicians to write
> 'if and only if' (it is also sometimes written as <=>). Since you didn't
> recognize it I guess I really need to write it out in full.
>
Oh, don't worry on my account. I am not a native speaker nor a
mathematician. You could leave iff and add [sic] to make everyone curious
and start googling "iff" :)
..Thomas
>
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>
