On Mon, 19 Sep 2022 18:45:35 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Thomas Stuefe has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> cjplummer feedback
>
> src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp line 187:
>
>> 185: }
>> 186:
>> 187: void print_with_childs(outputStream* st, BranchTracker& branchtracker,
>
> I know this isn't part of your changes, but the plural of child is children,
> not childs. However, I think child_nodes would probably be best here.
Fixed
> src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp line 320:
>
>> 318: return (loader_klass() != NULL) && (loader_klass() ==
>> target_node->loader_klass()) &&
>> 319: ((loader_name_oop() == NULL &&
>> target_node->loader_name_oop() == NULL) ||
>> 320: (::strcmp(loader_name(), target_node->loader_name()) ==
>> 0));
>
> This is hard to parse. I think using if/else statements would be cleaner.
Fixed
> test/hotspot/jtreg/serviceability/dcmd/vm/ClassLoaderHierarchyTest.java line
> 124:
>
>> 122: output.shouldContain(" +-- \"Kevin\",
>> ClassLoaderHierarchyTest$TestClassLoader");
>> 123: output.shouldContain(" +--
>> ClassLoaderHierarchyTest$TestClassLoader");
>> 124: output.shouldContain(" | +-- \"Bill\",
>> ClassLoaderHierarchyTest$TestClassLoader");
>
> Can you test using one long string, and do the same for each section below? I
> think embedding \\R as the newline character should work on all platforms. If
> you run into unforeseen issues, then don't worry about.
I'd prefer to leave it this way. I tried what you suggested before, but the
output can contain trailing spaces in the branchwork of the tree - that is a
byproduct of the indentation writer (the class writing the branches) and not
easy to fix. I wanted the test to be somewhat tolerant against minor future
changes in how the tree is printed.
-------------
PR: https://git.openjdk.org/jdk/pull/10312
