On Wed, 22 Jul 2026 19:09:24 GMT, Chen Liang <[email protected]> wrote:
>> A few fail-safe convenience methods added in #23548 are missing necessary >> null checks. In addition, the `PackageEntry` symbolic conversion and testing >> methods are not correctly rejecting `PackageDesc` representing the unnamed >> package, a deliberate choice when `PackageDesc` was added to Java. >> >> Luckily, these oversights can be easily fixed and verified with minor >> enhancements to existing tests. There are also accompanying minor doc >> enhancements about the situation. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Chen Liang 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 seven additional commits since > the last revision: > > - More coverage > - Merge branch 'master' of https://github.com/openjdk/jdk into > fix/cf-match-inconsistencies > - Stage > - Merge branch 'master' of https://github.com/openjdk/jdk into > fix/cf-match-inconsistencies > - Replicable pool states > - Wording tweak > - Match method null check and inconsistencies Reviewed — no blockers. Suggestions are inline. _— qwen3.7-max via Qwen Code /review_ src/java.base/share/classes/java/lang/classfile/attribute/ModulePackagesAttribute.java line 43: > 41: import jdk.internal.classfile.impl.TemporaryConstantPool; > 42: import jdk.internal.classfile.impl.UnboundAttribute; > 43: import jdk.internal.classfile.impl.Util; **[Suggestion]** The `TemporaryConstantPool` import at line 41 is now unused — this change replaced its only use (the manual loop in `ofNames(List<PackageDesc>)`) with `Util.packageEntryList(packages)`. Consider removing the stale import alongside the new `Util` import added here. src/java.base/share/classes/jdk/internal/classfile/impl/AbstractPoolEntry.java line 710: > 708: } > 709: return ref1.equalsString(desc.internalName()); > 710: } **[Suggestion]** Add `requireNonNull(desc)` here for consistency with the rest of this change. This PR adds `requireNonNull(desc)` to `ClassEntryImpl.matches(ClassDesc)`, `Utf8EntryImpl.isFieldType`, and `Utf8EntryImpl.isMethodType`, but not to this method or the sibling `ModuleEntryImpl.matches(ModuleDesc)` (~line 745). Null arguments still throw NPE via the immediate `desc.internalName()` / `desc.name()` dereference, so this is not a correctness problem — but the NPE is a bare dereference exception instead of the descriptive `requireNonNull` message the sibling methods now produce. **Suggested fix:** public boolean matches(PackageDesc desc) { requireNonNull(desc); if (desc.internalName().isEmpty()) { (and the same in `ModuleEntryImpl.matches(ModuleDesc)`) ------------- PR Review: https://git.openjdk.org/jdk/pull/31925#pullrequestreview-4833757204 PR Review Comment: https://git.openjdk.org/jdk/pull/31925#discussion_r3694697957 PR Review Comment: https://git.openjdk.org/jdk/pull/31925#discussion_r3694697954
