On Mon, 14 Sep 2020 18:35:29 GMT, Daniel D. Daugherty <dcu...@openjdk.org>
wrote:
>> @kimbarrett:
>>
>>> src/hotspot/share/oops/weakHandle.cpp
>>> 36 WeakHandle::WeakHandle(OopStorage* storage, oop obj) :
>>> 37 _obj(storage->allocate()) {
>>> 38 assert(obj != NULL, "no need to create weak null oop");
>>>
>>> Please format this differently so the ctor-init-list is more easily
>>> distinguished from the body. I don't care that much which of the several
>>> alternatives is used.
>>
>> After discussion with Erik, I changed the indent on L37 from two space to
>> four spaces.
>
> @kimbarrett
>
>> src/hotspot/share/runtime/objectMonitor.cpp
>> 244 // Check that object() and set_object() are called from the right
>> context:
>> 245 static void check_object_context() {
>>
>> This seems like checking we would normally only do in a debug build. Is this
>> really supposed to be done in product builds too? (It's written to support
>> that, just wondering if that's really what we want.) Maybe these aren't
>> called very often so it doesn't matter? I also see that guarantee (rather
>> than assert) is used a fair amount in this and related code.
>
> I've changed check_object_context() to only be defined and called
> when ASSERT is defined. I've also changed the guarantee() calls
> to assert() calls.
>
> I've done a couple of Mach5 Tier[1-8] test cycles on this code so I'm
> no longer worried about this code or its callers in release bits.
@kimbarrett
> src/hotspot/share/runtime/objectMonitor.cpp
> 249 guarantee(self->is_Java_thread() || self->is_VM_thread(), "must be");
> 250 if (self->is_Java_thread()) {
>
> Maybe instead
>
> if (self->is_Java_thread()) {
> ...
> } else {
> guarantee(self->is_VM_thread(), "must be");
> }
I've made this refactoring change, tweaked the comments above
the block a bit and switched from guarantee() to assert().
-------------
PR: https://git.openjdk.java.net/jdk/pull/135
