On Thu, 4 May 2023 20:55:46 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> It'd be nice to do even more factoring + renaming.
>> The lines 2326-2345 can be refactored to a function:
>>
>> bool StackRootCollector::report_native_frame_refs(jmethodID method) {
>> _blk->set_context(_thread_tag, _tid, _depth, method);
>> if (_is_top_frame) {
>> // JNI locals for the top frame.
>> assert(_java_thread != nullptr, "sanity");
>> _java_thread->active_handles()->oops_do(_blk);
>> if (_blk->stopped()) {
>> return false;
>> }
>> } else {
>> if (_last_entry_frame != nullptr) {
>> // JNI locals for the entry frame
>> assert(_last_entry_frame->is_entry_frame(), "checking");
>>
>> _last_entry_frame->entry_frame_call_wrapper()->handles()->oops_do(_blk);
>> if (_blk->stopped()) {
>> return false;
>> }
>> }
>> }
>> return true;
>> }
>>
>>
>> The function `report_stack_refs` can be renamed to `report_java_frame_refs`
>> to make function name more consistent.
>
> JNI local reporting uses this tricky _is_top_frame/_last_entry_frame stuff
> I think it would be better to have it in the main do_frame method for better
> readability
Sorry, I do not see how this improves readability.
Big functions with many layered conditions do not improve readability.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1185718941
