Btw congrats for finally getting JEP 352 to move on. That really took a
while.
Cheers, Thomas
On Thu, Mar 28, 2019 at 4:22 PM Thomas Stüfe <thomas.stu...@gmail.com>
wrote:
>
>
> 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
>>
>
