On Thu, 29 Aug 2024 08:52:03 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> David Holmes has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 13 additional >> commits since the last revision: >> >> - Merge branch 'master' into 8338257-utf8-length >> - Extra assertion requested by tstuefe >> - more missing casts >> - fix cast >> - missing cast >> - Fix incorrect comments and size_t use per Dean's review >> - Add missing cast for signed-to-unsigned converion. >> - unnecessary cast >> - Fix comments >> - Fix off-by-one error >> - ... and 3 more: https://git.openjdk.org/jdk/compare/e81bf3da...9dce4ffb > > src/hotspot/share/classfile/javaClasses.hpp line 138: > >> 136: // Legacy variants that truncate the length if needed >> 137: static int utf8_length_as_int(oop java_string); >> 138: static int utf8_length_as_int(oop java_string, typeArrayOop >> string_value); > > I don't get the point of this variant of the function. It takes a string and > a typearray. What is the contract here, is the only value allowed for > typearray the array oop underlying the string? If yes, why do you assert for > value equality in the function? That implies I can feed in any typearrayoop > here as long as it has the same value as the string. > > I can only see a single point where this function is used, so that does not > explain much. Maybe I am overlooking something, but why not just inline the > code into that one using call site? The as_int versions mirror the existing not-as-int versions. I admit I don't really see the point of unwrapping the array from the string but then pass them both. I assume they are intended/required to be a matching pair. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20560#discussion_r1736113281
