On Fri, 1 Aug 2025 15:49:57 GMT, Chen Liang <li...@openjdk.org> wrote:
> Provide a general facility for our null check APIs like
> Objects::requireNonNull or future Checks::nullCheck (void), converting the
> existing infrastructure to start tracking from a given stack site (depth
> offset) and a given stack slot (offset value).
VM side seems reasonable. Java side looks a bit clunky but that is up to
core-libs folk to evaluate.
General note: if comments start with a capital they should end with a period,
and vice-versa. Extended comments should consist of full sentences.
Thanks.
src/hotspot/share/classfile/javaClasses.cpp line 3078:
> 3076: do {
> 3077: // No backtrace available.
> 3078: if (!iter.repeat()) return false;
Suggestion:
if (!iter.repeat()) {
return false;
}
src/hotspot/share/interpreter/bytecodeUtils.cpp line 346:
> 344:
> 345: if (found && is_parameter) {
> 346: // Check MethodParameters for a name, if it carries a name
Suggestion:
// check MethodParameters for a name, if it carries a name
src/hotspot/share/interpreter/bytecodeUtils.cpp line 354:
> 352: char *var = cp->symbol_at(elem.name_cp_index)->as_C_string();
> 353: os->print("%s", var);
> 354:
No need for blank line
src/hotspot/share/interpreter/bytecodeUtils.cpp line 1483:
> 1481: // Is an explicit slot given?
> 1482: bool explicit_search = slot >= 0;
> 1483: if (explicit_search) {
Suggestion:
if (slot >= 0) {
No need for the temporary local.
src/hotspot/share/interpreter/bytecodeUtils.hpp line 40:
> 38: // Slot can be nonnegative to indicate an explicit search for the
> source of null
> 39: // If slot is negative (default), also search for the action that
> caused the NPE before
> 40: // deriving the actual slot and source of null by code parsing
Suggestion:
// Slot can be nonnegative to indicate an explicit search for the source of
null.
// If slot is negative (default), also search for the action that caused the
NPE before
// deriving the actual slot and source of null by code parsing.
Periods at the end of sentences in comments.
src/java.base/share/classes/java/lang/NullPointerException.java line 78:
> 76: NullPointerException(int stackOffset, int searchSlot) {
> 77: extendedMessageState = setupCustomBackTrace(stackOffset,
> searchSlot);
> 78: this();
I thought `this()` had to come first in a constructor? Is this new in 25 or 26?
src/java.base/share/classes/java/lang/NullPointerException.java line 101:
> 99: SEARCH_SLOT_MASK = SEARCH_SLOT_MAX << SEARCH_SLOT_SHIFT;
> 100:
> 101: // Access these fields in object monitor only
Suggestion:
// Access these fields only while holding this object's monitor lock.
src/java.base/share/classes/java/lang/NullPointerException.java line 162:
> 160: /// and the search slot will be derived by bytecode tracing. The
> message
> 161: /// will also include the action that caused the NPE besides the
> source of
> 162: /// the `null`.
Suggestion:
/// will also include the action that caused the NPE along with the
source
/// of the `null`.
-------------
PR Review: https://git.openjdk.org/jdk/pull/26600#pullrequestreview-3087045581
PR Review Comment: https://git.openjdk.org/jdk/pull/26600#discussion_r2253509165
PR Review Comment: https://git.openjdk.org/jdk/pull/26600#discussion_r2253517408
PR Review Comment: https://git.openjdk.org/jdk/pull/26600#discussion_r2253519550
PR Review Comment: https://git.openjdk.org/jdk/pull/26600#discussion_r2253472224
PR Review Comment: https://git.openjdk.org/jdk/pull/26600#discussion_r2253481631
PR Review Comment: https://git.openjdk.org/jdk/pull/26600#discussion_r2253558481
PR Review Comment: https://git.openjdk.org/jdk/pull/26600#discussion_r2253541806
PR Review Comment: https://git.openjdk.org/jdk/pull/26600#discussion_r2253487877
