On Wed, 11 Feb 2026 22:07:10 GMT, Roger Riggs <[email protected]> wrote:
>> David Beaumont has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Updated copyright
>> - Feedback changes
>
> src/hotspot/share/classfile/classLoader.cpp line 269:
>
>> 267: static JImageFile* jimage_non_null() {
>> 268: assert(jimage_exists(), "should have been opened by
>> ClassLoader::lookup_vm_options "
>> 269: "and remained throughout normal JVM
>> lifetime");
>
> "remained" -> "remain open"
Done.
> src/hotspot/share/runtime/arguments.cpp line 2995:
>
>> 2993:
>> 2994: // Called after ClassLoader::lookup_vm_options() but before class
>> loading begins.
>> 2995: // TODO: Obtain and pass correct preview mode flag value here.
>
> Suggestion: A hotspot convention is to include the bugid of the
> fix/enhancement that is pending.
> Since you've planned the progression of PR's that may be useful here. YMMV.
And I've also been told at least once that we're not to add bug numbers into
code. I don't have the other PR created yet either, and it will all be forced
when the other code arrives due to tests anyway.
> src/java.base/share/classes/jdk/internal/jimage/ModuleReference.java line 85:
>
>> 83: return new ModuleReference(moduleName, previewFlag(isPreview));
>> 84: }
>> 85:
>
> A one-line comment may be useful here.
Done.
> src/java.base/share/native/libjimage/jimage.cpp line 112:
>
>> 110: size_t module_name_len = strlen(module_name);
>> 111: size_t name_len = strlen(name);
>> 112: size_t preview_infix_len = strlen(preview_infix);
>
> `sizeof` would make this a constant.
I specifically asked about this and was told the compiler will also inline the
strlen as expected.
I dislike using sizeof because it's "off by one" and needs you to account for
nul char, when what's conceptually wanted is the "string length" (i.e. strlen).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2923873972
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2923879942
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2923978667
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2924042081
