-
Notifications
You must be signed in to change notification settings - Fork 5.8k
JDK-8301076: Replace NULL with nullptr in share/prims/ #12188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
👋 Welcome back jsjolen! A progress list of the required criteria for merging this PR into |
@jdksjolen The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
039ee82
to
b33f356
Compare
src/hotspot/share/prims/jni.cpp
Outdated
size_t length = java_lang_String::utf8_length(java_string, s_value); | ||
/* JNI Specification states return NULL on OOM */ | ||
/* JNI Specification states return nullptr on OOM */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null
src/hotspot/share/prims/jni.cpp
Outdated
*isCopy = JNI_FALSE; \ | ||
} \ | ||
/* Empty array: legal but useless, can't return NULL. \ | ||
/* Empty array: legal but useless, can't return nullptr. \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null
src/hotspot/share/prims/jni.cpp
Outdated
* Return a pointer to something useless. \ | ||
* Avoid asserts in typeArrayOop. */ \ | ||
result = (ElementType*)get_bad_address(); \ | ||
} else { \ | ||
/* JNI Specification states return NULL on OOM */ \ | ||
/* JNI Specification states return nullptr on OOM */ \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null
src/hotspot/share/prims/jni.cpp
Outdated
} else { | ||
// Inflate latin1 encoded string to UTF16 | ||
typeArrayOop s_value = java_lang_String::value(s); | ||
int s_len = java_lang_String::length(s, s_value); | ||
ret = NEW_C_HEAP_ARRAY_RETURN_NULL(jchar, s_len + 1, mtInternal); // add one for zero termination | ||
/* JNI Specification states return NULL on OOM */ | ||
if (ret != NULL) { | ||
/* JNI Specification states return nullptr on OOM */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null
src/hotspot/share/prims/jniCheck.cpp
Outdated
@@ -92,7 +92,7 @@ static struct JNINativeInterface_ * unchecked_jni_NativeInterface; | |||
extern "C" { \ | |||
result_type JNICALL header { \ | |||
Thread* cur = Thread::current_or_null(); \ | |||
if (cur == NULL || !cur->is_Java_thread()) { \ | |||
if (cur == nullptr || !cur->is_Java_thread()) { \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
align
@@ -932,7 +932,7 @@ JNI_ENTRY_CHECKED(ResultType, \ | |||
functionEnter(thr); \ | |||
va_list args; \ | |||
IN_VM( \ | |||
jniCheck::validate_call(thr, NULL, methodID, obj); \ | |||
jniCheck::validate_call(thr, nullptr, methodID, obj); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
align
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not aligned from start, not fixing.
@@ -950,7 +950,7 @@ JNI_ENTRY_CHECKED(ResultType, \ | |||
va_list args)) \ | |||
functionEnter(thr); \ | |||
IN_VM(\ | |||
jniCheck::validate_call(thr, NULL, methodID, obj); \ | |||
jniCheck::validate_call(thr, nullptr, methodID, obj); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
align
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not aligned from start, not fixing.
@@ -966,7 +966,7 @@ JNI_ENTRY_CHECKED(ResultType, \ | |||
const jvalue * args)) \ | |||
functionEnter(thr); \ | |||
IN_VM( \ | |||
jniCheck::validate_call(thr, NULL, methodID, obj); \ | |||
jniCheck::validate_call(thr, nullptr, methodID, obj); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
align
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not aligned from start, not fixing.
Passes tier1. |
Webrevs
|
// Utility macro that checks for NULL pointers: | ||
#define NULL_CHECK(X, Y) if ((X) == NULL) { return (Y); } | ||
// Utility macro that checks for null pointers: | ||
#NULL nullptr_CHECK(X, Y) if ((X) == nullptr) { return (Y); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
causes compilation error
} | ||
|
||
bool JvmtiEnvThreadState::is_virtual() { | ||
return _state->is_virtual(); | ||
} | ||
|
||
// Use _thread_saved if cthread is detached from JavaThread (_thread == NULL). | ||
// Use _thread_saved if cthread is detached from JavaThread (_thread == null). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/null/nullptr/ or s/==/is/
assert(e1 != NULL, "e1 != NULL"); | ||
assert(e2 != NULL, "e2 != NULL"); | ||
assert(e1 != nullptr, "e1 != null"); | ||
assert(e2 != nullptr, "e2 != null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/null/nullptr/ ?
@@ -177,7 +177,7 @@ void GrowableCache::append(GrowableElement* e) { | |||
// remove the element at index | |||
void GrowableCache::remove (int index) { | |||
GrowableElement *e = _elements->at(index); | |||
assert(e != NULL, "e != NULL"); | |||
assert(e != nullptr, "e != null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/null/nullptr/ ?
@@ -159,7 +159,7 @@ int GrowableCache::length() { | |||
// get the value of the index element in the collection | |||
GrowableElement* GrowableCache::at(int index) { | |||
GrowableElement *e = (GrowableElement *) _elements->at(index); | |||
assert(e != NULL, "e != NULL"); | |||
assert(e != nullptr, "e != null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/null/nullptr/ ?
_jvmti_breakpoints = new JvmtiBreakpoints(listener_fun); | ||
assert(_jvmti_breakpoints != NULL, "_jvmti_breakpoints != NULL"); | ||
assert(_jvmti_breakpoints != nullptr, "_jvmti_breakpoints != null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/null/nullptr/ ?
return (*_jvmti_breakpoints); | ||
} | ||
|
||
void JvmtiCurrentBreakpoints::listener_fun(void *this_obj, address *cache) { | ||
JvmtiBreakpoints *this_jvmti = (JvmtiBreakpoints *) this_obj; | ||
assert(this_jvmti != NULL, "this_jvmti != NULL"); | ||
assert(this_jvmti != nullptr, "this_jvmti != null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/null/nullptr/ ?
@@ -1043,7 +1043,7 @@ static bool advertise_con_value(int which) { | |||
|
|||
#undef ONE_PLUS | |||
#undef VALUE_COMMA | |||
#undef STRING_NULL | |||
#undef STRING_nullptr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be changed.
@@ -1021,8 +1021,8 @@ void MethodHandles::trace_method_handle_interpreter_entry(MacroAssembler* _masm, | |||
static const int con_value_count = EACH_NAMED_CON(ONE_PLUS, IGNORE_REQ) 0; | |||
#define VALUE_COMMA(scope,value) scope::value, | |||
static const int con_values[con_value_count+1] = { EACH_NAMED_CON(VALUE_COMMA, IGNORE_REQ) 0 }; | |||
#define STRING_NULL(scope,value) #value "\0" | |||
static const char con_names[] = { EACH_NAMED_CON(STRING_NULL, IGNORE_REQ) }; | |||
#define STRING_nullptr(scope,value) #value "\0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be changed.
#define STRING_NULL(scope,value) #value "\0" | ||
static const char con_names[] = { EACH_NAMED_CON(STRING_NULL, IGNORE_REQ) }; | ||
#define STRING_nullptr(scope,value) #value "\0" | ||
static const char con_names[] = { EACH_NAMED_CON(STRING_nullptr, IGNORE_REQ) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be changed.
@@ -97,7 +97,7 @@ JavaThread* UpcallLinker::on_entry(UpcallStub::FrameData* context) { | |||
context->old_handles = thread->active_handles(); | |||
|
|||
// For the profiler, the last_Java_frame information in thread must always be in | |||
// legal state. We have no last Java frame if last_Java_sp == NULL so | |||
// legal state. We have no last Java frame if last_Java_sp == null so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/null/nullptr/ ?
Thanks for these review comments, I've fixed your suggestions. I must've accidentally submitted a different branch for tier1 testing, as this doesn't compile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@jdksjolen This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 105 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few adjustments but overall looks good. Thanks.
src/hotspot/share/prims/jni.cpp
Outdated
int s_len = java_lang_String::length(s, s_value); | ||
bool is_latin1 = java_lang_String::is_latin1(s); | ||
buf = NEW_C_HEAP_ARRAY_RETURN_NULL(jchar, s_len + 1, mtInternal); // add one for zero termination | ||
/* JNI Specification states return NULL on OOM */ | ||
if (buf != NULL) { | ||
/* JNI Specification states return nullptr on OOM */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null
@@ -53,7 +53,7 @@ | |||
// | |||
// collector.collect(); | |||
// JvmtiCodeBlobDesc* blob = collector.first(); | |||
// while (blob != NULL) { | |||
// while (blob != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullptr
src/hotspot/share/prims/jvmtiEnv.cpp
Outdated
if (event_thread == NULL) { | ||
// Can be called at Agent_OnLoad() time with event_thread == NULL | ||
if (event_thread == nullptr) { | ||
// Can be called at Agent_OnLoad() time with event_thread == null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullptr
@@ -167,31 +167,31 @@ class JvmtiEnvBase : public CHeapObj<mtInternal> { | |||
return byte_offset_of(JvmtiEnvBase, _jvmti_external); | |||
}; | |||
|
|||
// If (thread == NULL) then return current thread object. | |||
// If (thread == null) then return current thread object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullptr
return false; | ||
} | ||
oop cur_obj = current->jvmti_vthread(); | ||
|
||
// cur_obj == NULL is true for normal platform threads only | ||
// cur_obj == null is true for normal platform threads only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullptr
// Use (thread == null) to enable/disable an event globally. | ||
// Use (thread != null) to enable/disable an event for a particular thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullptr
All tests passing in GHA. @alexmenkov , would you mind approving :)? |
Tier1 is passing and this has 2 approvals, I'm integrating. Thank you for the reviews. /integrate |
Going to push as commit b76a52f.
Your commit was automatically rebased without conflicts. |
@jdksjolen Pushed as commit b76a52f. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory share/prims/. 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:
An example of this:
Note how
nullptr
participates in a code expression here, we really are talking about the specific valuenullptr
.Thanks!
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12188/head:pull/12188
$ git checkout pull/12188
Update a local copy of the PR:
$ git checkout pull/12188
$ git pull https://git.openjdk.org/jdk pull/12188/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12188
View PR using the GUI difftool:
$ git pr show -t 12188
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12188.diff