RFR: JDK-8301223: Replace NULL with nullptr in share/gc/g1/
Thomas Schatzl
tschatzl at openjdk.org
Tue Apr 18 10:06:04 UTC 2023
On Fri, 27 Jan 2023 10:06:10 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:
> Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory share/gc/g1/. Unfortunately the script that does the change isn't perfect, and so we
> need to comb through these manually to make sure nothing has gone wrong. I also review these changes but things slip past my eyes sometimes.
>
> Here are some typical things to look out for:
>
> 1. No changes but copyright header changed (probably because I reverted some changes but forgot the copyright).
> 2. Macros having their NULL changed to nullptr, these are added to the script when I find them. They should be NULL.
> 3. nullptr in comments and logs. We try to use lower case "null" in these cases as it reads better. An exception is made when code expressions are in a comment.
>
> An example of this:
>
> ```c++
> // This function returns null
> void* ret_null();
> // This function returns true if *x == nullptr
> bool is_nullptr(void** x);
>
>
> Note how `nullptr` participates in a code expression here, we really are talking about the specific value `nullptr`.
>
> Thanks!
Changes requested by tschatzl (Reviewer).
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 46:
> 44: const Type **fields = TypeTuple::fields(2);
> 45: fields[TypeFunc::Parms+0] = TypeInstPtr::NOTnullptr; // original field value
> 46: fields[TypeFunc::Parms+1] = TypeRawPtr::NOTnullptr; // thread
Suggestion:
fields[TypeFunc::Parms+0] = TypeInstPtr::NOTNULL; // original field value
fields[TypeFunc::Parms+1] = TypeRawPtr::NOTNULL; // thread
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 59:
> 57: const Type **fields = TypeTuple::fields(2);
> 58: fields[TypeFunc::Parms+0] = TypeRawPtr::NOTnullptr; // Card addr
> 59: fields[TypeFunc::Parms+1] = TypeRawPtr::NOTnullptr; // thread
Suggestion:
fields[TypeFunc::Parms+0] = TypeRawPtr::NOTNULL; // Card addr
fields[TypeFunc::Parms+1] = TypeRawPtr::NOTNULL; // thread
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 77:
> 75: *
> 76: * If the previous value is nullptr there is no need to save the old value.
> 77: * References that are nullptr are filtered during runtime by the barrier
Suggestion:
* If the previous value is null there is no need to save the old value.
* References that are null are filtered during runtime by the barrier
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 81:
> 79: *
> 80: * However in the case of newly allocated objects it might be possible to
> 81: * prove that the reference about to be overwritten is nullptr during compile
Suggestion:
* prove that the reference about to be overwritten is null during compile
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 251:
> 249: }
> 250:
> 251: // if (pre_val != null)
Suggestion:
// if (pre_val != nullptr)
This is converted wrongly in a comment
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 253:
> 251: // if (pre_val != null)
> 252: __ if_then(pre_val, BoolTest::ne, kit->null()); {
> 253: Node* buffer = __ load(__ ctrl(), buffer_adr, TypeRawPtr::NOTnullptr, T_ADDRESS, Compile::AliasIdxRaw);
Suggestion:
Node* buffer = __ load(__ ctrl(), buffer_adr, TypeRawPtr::NOTNULL, T_ADDRESS, Compile::AliasIdxRaw);
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 273:
> 271: __ make_leaf_call(tf, CAST_FROM_FN_PTR(address, G1BarrierSetRuntime::write_ref_field_pre_entry), "write_ref_field_pre_entry", pre_val, tls);
> 272: } __ end_if(); // (!index)
> 273: } __ end_if(); // (pre_val != null)
Suggestion:
} __ end_if(); // (pre_val != nullptr)
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 291:
> 289: * To reduce the number of updates to the remembered set the post-barrier
> 290: * filters updates to fields in objects located in the Young Generation,
> 291: * the same region as the reference, when the nullptr is being written or
Suggestion:
* the same region as the reference, when null is being written or
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 382:
> 380: // If we are writing a null then we need no post barrier
> 381:
> 382: if (val != nullptr && val->is_Con() && val->bottom_type() == TypePtr::nullptr_PTR) {
Suggestion:
if (val != nullptr && val->is_Con() && val->bottom_type() == TypePtr::NULL_PTR) {
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 385:
> 383: // Must be null
> 384: const Type* t = val->bottom_type();
> 385: assert(t == Type::TOP || t == TypePtr::nullptr_PTR, "must be null");
Suggestion:
assert(t == Type::TOP || t == TypePtr::NULL_PTR, "must be null");
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 386:
> 384: const Type* t = val->bottom_type();
> 385: assert(t == Type::TOP || t == TypePtr::nullptr_PTR, "must be null");
> 386: // No post barrier if writing nullx
Suggestion:
// No post barrier if writing null.
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 437:
> 435: // potentially reset these fields in the JavaThread.
> 436: Node* index = __ load(__ ctrl(), index_adr, TypeX_X, TypeX_X->basic_type(), Compile::AliasIdxRaw);
> 437: Node* buffer = __ load(__ ctrl(), buffer_adr, TypeRawPtr::NOTnullptr, T_ADDRESS, Compile::AliasIdxRaw);
Suggestion:
Node* buffer = __ load(__ ctrl(), buffer_adr, TypeRawPtr::NOTNULL, T_ADDRESS, Compile::AliasIdxRaw);
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 462:
> 460: __ if_then(xor_res, BoolTest::ne, zeroX, likely); {
> 461:
> 462: // No barrier if we are storing a null
Suggestion:
// No barrier if we are storing a null.
src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 342:
> 340: do { \
> 341: assert_at_safepoint(); \
> 342: assert(Thread::current_or_null() != nullptr, "no current thread"); \
Suggestion:
assert(Thread::current_or_null() != nullptr, "no current thread"); \
src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 509:
> 507: // to support an allocation of the given "word_size". If
> 508: // successful, perform the allocation and return the address of the
> 509: // allocated block, or else "null".
Suggestion:
// allocated block, or else null.
src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 1921:
> 1919: assert(limit == bottom,
> 1920: "the region limit should be at bottom");
> 1921: // we return null and the caller should try calling
Suggestion:
// We return null and the caller should try calling
src/hotspot/share/gc/g1/g1FreeIdSet.cpp line 36:
> 34: G1FreeIdSet::G1FreeIdSet(uint start, uint size) :
> 35: _sem(size), // counting semaphore for available ids
> 36: _next(nullptr), // array of "next" indices
Suggestion:
_next(nullptr), // array of "next" indices
src/hotspot/share/gc/g1/g1RemSet.cpp line 1502:
> 1500:
> 1501: // If the card is no longer dirty, nothing to do.
> 1502: // We cannot load the card value before the "r == null" check, because G1
Suggestion:
// We cannot load the card value before the "r == nullptr" check above, because G1
-------------
PR Review: https://git.openjdk.org/jdk/pull/12248#pullrequestreview-1274727932
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090343260
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090343530
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090341235
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090341394
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090342158
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090342475
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090342663
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090344299
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090344705
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090345163
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090345368
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090345759
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090346018
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090348911
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090349649
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090351098
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090353074
PR Review Comment: https://git.openjdk.org/jdk/pull/12248#discussion_r1090356716
More information about the hotspot-dev
mailing list