On Tue, 16 Apr 2024 06:13:59 GMT, Dean Long <dl...@openjdk.org> wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Union fields which usages do not overlap
>
> src/hotspot/share/code/nmethod.cpp line 1235:
>
>> 1233: int skipped_insts_size =
>> code_buffer->total_skipped_instructions_size();
>> 1234: #ifdef ASSERT
>> 1235: assert(((skipped_insts_size >> 16) == 0), "size is bigger than
>> 64Kb: %d", skipped_insts_size);
>
> Suggestion:
>
>
> I think it's simpler just to use checked_cast below.
Agree
> src/hotspot/share/code/nmethod.cpp line 1240:
>
>> 1238: int consts_offset =
>> code_buffer->total_offset_of(code_buffer->consts());
>> 1239: assert(consts_offset == 0, "const_offset: %d", consts_offset);
>> 1240: #endif
>
> Suggestion:
I assume you are suggesting to remove `#ifdef ASSERT`.
Done.
> src/hotspot/share/code/nmethod.cpp line 1241:
>
>> 1239: assert(consts_offset == 0, "const_offset: %d", consts_offset);
>> 1240: #endif
>> 1241: _skipped_instructions_size = (uint16_t)skipped_insts_size;
>
> Suggestion:
>
> _skipped_instructions_size =
> checked_cast<uint16_t>(code_buffer->total_skipped_instructions_size());
Done.
> src/hotspot/share/code/nmethod.cpp line 1441:
>
>> 1439: int deps_size = align_up((int)dependencies->size_in_bytes(),
>> oopSize);
>> 1440: int sum_size = oops_size + metadata_size + deps_size;
>> 1441: assert((sum_size >> 16) == 0, "data size is bigger than 64Kb: %d",
>> sum_size);
>
> I suggest using checked_cast for the assignment below, rather than
> special-purpose checks here.
Okay. But I will put above code under `#ifdef ASSERT` then.
> src/hotspot/share/code/nmethod.cpp line 1445:
>
>> 1443: _metadata_offset = (uint16_t)oops_size;
>> 1444: _dependencies_offset = _metadata_offset +
>> (uint16_t)metadata_size;
>> 1445: _scopes_pcs_offset = _dependencies_offset +
>> (uint16_t)deps_size;
>
> Use checked_cast instead of raw casts.
okay
> src/hotspot/share/code/nmethod.cpp line 1459:
>
>> 1457: assert((data_offset() + data_end_offset) <= nmethod_size, "wrong
>> nmethod's size: %d < %d", nmethod_size, (data_offset() + data_end_offset));
>> 1458:
>> 1459: _entry_offset =
>> (uint16_t)offsets->value(CodeOffsets::Entry);
>
> Use checked_cast.
done
> src/hotspot/share/memory/heap.hpp line 58:
>
>> 56: void set_length(size_t length) {
>> 57: LP64_ONLY( assert(((length >> 32) == 0), "sanity"); )
>> 58: _header._length = (uint32_t)length;
>
> Suggestion:
>
> _header._length = checked_cast<uint32_t>length;
Done.
> src/hotspot/share/memory/heap.hpp line 63:
>
>> 61: // Accessors
>> 62: void* allocated_space() const { return (void*)(this +
>> 1); }
>> 63: size_t length() const { return
>> (size_t)_header._length; }
>
> This cast looks unnecessary.
Agree.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567619512
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567620520
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567620735
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567627565
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567636013
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567638682
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567644204
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567645140
