On Tue, 5 Aug 2025 07:55:41 GMT, David Holmes <dhol...@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).
>
> 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.
If we don't adopt the enum I suggested, then I do prefer the naming of the
condition through a variable. It gives you a hint of what the check is looking
for, and not only what the check does.
> 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.
I'd like to see the description for `slot` pushed into an enum, and make any
negative number except `-1` explicitly forbidden.
```c++
enum class NPESlot : int {
// docs here
None = -1,
// docs here
Explicit // Anything >= 0
};
bool get_NPE_message_at(outputStream* ss, Method* method, int bci, NPESlot
slot) {
if (slot != NPESlot::None) {
// Explicit search
}
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26600#discussion_r2253888922
PR Review Comment: https://git.openjdk.org/jdk/pull/26600#discussion_r2253865251
