8299974: Replace NULL with nullptr in share/adlc/ #14008

jdksjolen · 2023-05-16T11:54:20Z

Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory share/adlc. 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:

No changes but copyright header changed (probably because I reverted some changes but forgot the copyright).
Macros having their NULL changed to nullptr, these are added to the script when I find them. They should be NULL.
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:

// 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!

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8299974: Replace NULL with nullptr in share/adlc/

Reviewers

Dean Long (@dean-long - Reviewer) ⚠️ Review applies to 46653faf
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14008/head:pull/14008
$ git checkout pull/14008

Update a local copy of the PR:
$ git checkout pull/14008
$ git pull https://git.openjdk.org/jdk.git pull/14008/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14008

View PR using the GUI difftool:
$ git pr show -t 14008

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14008.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-16T11:55:13Z

👋 Welcome back jsjolen! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-05-16T11:56:41Z

@jdksjolen The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

jdksjolen

Mostly alignment and comment issues.

jdksjolen · 2023-05-16T11:54:55Z

src/hotspot/share/adlc/adlArena.cpp

@@ -53,7 +53,7 @@ void  AdlChunk::operator delete(void* p, size_t length) {
 }

 AdlChunk::AdlChunk(size_t length) {
-  _next = NULL;         // Chain on the linked list
+  _next = nullptr;         // Chain on the linked list


jdksjolen · 2023-05-16T11:55:06Z

src/hotspot/share/adlc/adlArena.cpp

-  _first = _chunk = NULL;       // Normal, new-arena initialization
-  _hwm = _max = NULL;
+  _first = _chunk = nullptr;       // Normal, new-arena initialization
+  _hwm = _max = nullptr;


jdksjolen · 2023-05-16T11:55:57Z

src/hotspot/share/adlc/adlparse.cpp

-      char *pType = NULL;        // parameter type
-      char *pName = NULL;        // parameter name
+      char *pType = nullptr;        // parameter type
+      char *pName = nullptr;        // parameter name


jdksjolen · 2023-05-16T11:56:07Z

src/hotspot/share/adlc/adlparse.cpp

@@ -970,7 +970,7 @@ void ADLParser::enc_class_parse_block(EncClass* encoding, char* ec_name) {
 //------------------------------frame_parse-----------------------------------
 void ADLParser::frame_parse(void) {
  FrameForm  *frame;              // Information about stack-frame layout
-  char       *desc = NULL;        // String representation of frame
+  char       *desc = nullptr;        // String representation of frame


jdksjolen · 2023-05-16T11:56:16Z

src/hotspot/share/adlc/adlparse.cpp

    parse_err(SYNERR, "missing frame pointer definition in frame section.\n");
    return;
  }
  // !!!!! !!!!!
-  // if(frame->_interpreter_frame_ptr_reg == NULL) {
+  // if(frame->_interpreter_frame_ptr_reg == null) {


jdksjolen · 2023-05-16T12:04:25Z

src/hotspot/share/adlc/formssel.cpp

@@ -2944,7 +2944,7 @@ ComponentList::ComponentList() : NameList(), _matchcnt(0) {
 ComponentList::~ComponentList() {
  // // This list may not own its elements if copied via assignment
  // Component *component;
-  // for (reset(); (component = iter()) != NULL;) {
+  // for (reset(); (component = iter()) != null;) {


jdksjolen · 2023-05-16T12:04:38Z

src/hotspot/share/adlc/formssel.cpp

@@ -3382,13 +3382,13 @@ const char *MatchNode::reduce_right(FormDict &globals) const {
    rightStr = mnode->_rChild->_internalop ? mnode->_rChild->_internalop
      : mnode->_rChild->_opType;
  }
-  // Else, May be simple chain rule: (Set dst operand_form), rightStr=NULL;
+  // Else, May be simple chain rule: (Set dst operand_form), rightStr=null;


jdksjolen · 2023-05-16T12:05:22Z

src/hotspot/share/adlc/output_c.cpp

-  //   MachNode  *inst0 = NULL;
+  //   MachNode  *inst0 = null;
  //   ...
-  //   MachNode  *instMAX = NULL;
+  //   MachNode  *instMAX = null;
  //


jdksjolen · 2023-05-16T12:06:25Z

src/hotspot/share/adlc/output_h.cpp

@@ -580,7 +580,7 @@ void gen_oper_format(FILE *fp, FormDict &globals, OperandForm &oper, bool for_c_
      // Default formats for base operands (RegI, RegP, ConI, ConP, ...)
      oper.ext_format(fp, globals, 0);
    }
-  } else { // oper._format == NULL
+  } else { // oper._format == null


jdksjolen · 2023-05-16T12:06:34Z

src/hotspot/share/adlc/output_h.cpp

@@ -1112,7 +1112,7 @@ void ArchDesc::declare_pipe_classes(FILE *fp_hpp) {
  fprintf(fp_hpp, "};\n\n");

 //  const char *classname;
-//  for (_pipeline->_classlist.reset(); (classname = _pipeline->_classlist.iter()) != NULL; ) {
+//  for (_pipeline->_classlist.reset(); (classname = _pipeline->_classlist.iter()) != null; ) {


jdksjolen · 2023-05-16T13:35:06Z

Hm. I think that this is completely broken. This is a compiler/code generation tool so any string containing null instead of NULL now is incorrect. I'll leave this until a bit later.

jdksjolen

Found all (most?) of the faulty NULL -> null conversions.

jdksjolen · 2023-05-24T16:31:46Z

src/hotspot/share/adlc/dfa.cpp

-      // child should be NULL, to distinguish from the predicated version.
-      fprintf(fp, " && _kids[1] == NULL");
+      // child should be null, to distinguish from the predicated version.
+      fprintf(fp, " && _kids[1] == null");


jdksjolen · 2023-05-24T16:32:23Z

src/hotspot/share/adlc/dfa.cpp

+  printf("%s", (_result     == null ? "null" : _result ) );
+  printf("%s", (_constraint == null ? "null" : _constraint ) );
+  printf("%s", (_valid      == null ? "null" : _valid ) );


jdksjolen · 2023-05-24T16:33:40Z

src/hotspot/share/adlc/formssel.cpp

    const char* opType = _matrule->_opType;
    if (strcmp(opType, "Set")==0)
      opType = _matrule->_rChild->_opType;
    if (strcmp(opType,"ThreadLocal")==0) {
      fprintf(stderr, "Warning: ThreadLocal instruction %s should be named 'tlsLoadP_*'\n",
-              (_ident == NULL ? "NULL" : _ident));
+              (_ident == nullptr ? "null" : _ident));


jdksjolen · 2023-05-24T16:36:46Z

src/hotspot/share/adlc/output_c.cpp

+  fprintf(fp_cpp, "static const Pipeline pipeline_class_Zero_Instructions(0, 0, true, 0, 0, false, false, false, false, null, null, null, Pipeline_Use(0, 0, 0, null));\n\n");
+  fprintf(fp_cpp, "static const Pipeline pipeline_class_Unknown_Instructions(0, 0, true, 0, 0, false, true, true, false, null, null, null, Pipeline_Use(0, 0, 0, null));\n\n");



jdksjolen · 2023-05-24T16:37:05Z

src/hotspot/share/adlc/output_c.cpp

@@ -889,7 +889,7 @@ void ArchDesc::build_pipe_classes(FILE *fp_cpp) {
        pipeline_reads_index+1);
    }
    else
-      fprintf(fp_cpp, " NULL,");
+      fprintf(fp_cpp, " null,");


jdksjolen · 2023-05-24T16:40:22Z

src/hotspot/share/adlc/output_c.cpp

  fprintf(_CPP_PIPELINE_file._fp, "const Pipeline * %*s::pipeline_class() { return %s; }\n",
-    max_ident_len, "Node", _pipeline ? "(&pipeline_class_Zero_Instructions)" : "NULL");
+    max_ident_len, "Node", _pipeline ? "(&pipeline_class_Zero_Instructions)" : "null");
  fprintf(_CPP_PIPELINE_file._fp, "const Pipeline * %*s::pipeline() const { return %s; }\n",
-    max_ident_len, "Node", _pipeline ? "(&pipeline_class_Zero_Instructions)" : "NULL");
+    max_ident_len, "Node", _pipeline ? "(&pipeline_class_Zero_Instructions)" : "null");
  fprintf(_CPP_PIPELINE_file._fp, "\n");

  fprintf(_CPP_PIPELINE_file._fp, "const Pipeline * %*s::pipeline_class() { return %s; }\n",
-    max_ident_len, "MachNode", _pipeline ? "(&pipeline_class_Unknown_Instructions)" : "NULL");
+    max_ident_len, "MachNode", _pipeline ? "(&pipeline_class_Unknown_Instructions)" : "null");
  fprintf(_CPP_PIPELINE_file._fp, "const Pipeline * %*s::pipeline() const { return pipeline_class(); }\n",


jdksjolen · 2023-05-24T16:40:49Z

src/hotspot/share/adlc/output_c.cpp

    const char *opEnumName = machOperEnum(iopn);
    // Generate the case statement for this opcode
    fprintf(fp_cpp, "  case %s:", opEnumName);
-    fprintf(fp_cpp, "    return NULL;\n");
+    fprintf(fp_cpp, "    return null;\n");


jdksjolen · 2023-05-24T16:40:53Z

src/hotspot/share/adlc/output_c.cpp

@@ -3912,15 +3912,15 @@ void ArchDesc::buildMachOperGenerator(FILE *fp_cpp) {
  fprintf(fp_cpp, "  }\n");

  // Generate the closing for method Matcher::MachOperGenerator
-  fprintf(fp_cpp, "  return NULL;\n");
+  fprintf(fp_cpp, "  return null;\n");


jdksjolen · 2023-05-24T16:41:04Z

src/hotspot/share/adlc/output_c.cpp

@@ -4171,7 +4171,7 @@ void ArchDesc::buildMachNodeGenerator(FILE *fp_cpp) {
  fprintf(fp_cpp, "  };\n");

  // Generate the closing for method Matcher::MachNodeGenerator
-  fprintf(fp_cpp, "  return NULL;\n");
+  fprintf(fp_cpp, "  return null;\n");


jdksjolen · 2023-05-24T16:41:40Z

src/hotspot/share/adlc/output_h.cpp

@@ -712,7 +712,7 @@ void gen_inst_format(FILE *fp, FormDict &globals, InstructForm &inst, bool for_c
  }
  else if( inst.is_ideal_mem() ) {
    // Print out the field name if available to improve readability
-    fprintf(fp,  "  if (ra->C->alias_type(adr_type())->field() != NULL) {\n");
+    fprintf(fp,  "  if (ra->C->alias_type(adr_type())->field() != null) {\n");


openjdk · 2023-05-24T17:22:46Z

@jdksjolen this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8299974
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

jdksjolen · 2023-05-24T17:55:16Z

Builds on linux-x64, running tier1. Open for review.

mlbridge · 2023-05-24T18:01:09Z

Webrevs

dean-long · 2023-05-24T18:27:11Z

src/hotspot/share/adlc/archDesc.cpp

@@ -85,9 +85,9 @@ void ChainList::dump() {



Missing copyright year update.

dean-long

Looks good, except I found one missing copyright update.

openjdk · 2023-05-24T18:56:36Z

@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:

8299974: Replace NULL with nullptr in share/adlc/

Reviewed-by: dlong, kvn

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 36 new commits pushed to the master branch:

27ba8bd: 8308108: Support Unicode extension for collation settings
89b3c37: 8307125: compiler/jvmci/compilerToVM/MaterializeVirtualObjectTest.java hits assert(!Continuation::is_frame_in_continuation(thread(), fr())) failed: No support for deferred values in continuations
98acce1: 8306703: JFR: Summary views
534de6d: 8300491: SymbolLookup::libraryLookup accepts strings with terminators
48d21bd: 8286597: Implement PollerProvider on AIX
e7edf8d: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters
90e57fd: 8308335: JFR: Remove @experimental from Virtual Threads events
7e2e05d: 8308098: G1: Remove redundant checks in G1ObjectCountIsAliveClosure
2599ada: 8308655: Narrow types of ConstantPool and ConstMethod returns
5a0a238: 8308746: C2 IR test failures for TestFpMinMaxReductions.java with SSE2
... and 26 more: https://git.openjdk.org/jdk/compare/2d4d850813235a7533cd3bbf776adf69f90f02e6...master

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 master branch, type /integrate in a new comment.

vnkozlov

I have few comments

vnkozlov · 2023-05-24T19:03:03Z

src/hotspot/share/adlc/adlparse.cpp

+  if( _curline == nullptr )          // Return null at EOF.
+    return nullptr;


Spacing of comment. Please, fix code style to:

if (_curline == nullptr) { // Return null at EOF. return nullptr; }

vnkozlov · 2023-05-24T19:05:13Z

src/hotspot/share/adlc/adlparse.cpp

  }
  _ptr = end;                     // Reset _ptr to point to next char after token

  // Make sure we do not try to use #defined identifiers.  If start is
-  // NULL an error was already reported.
-  if (do_preproc && start != NULL) {
+  // null an error was already reported.


May be use nullptr here since we are talking about value in start variable.

vnkozlov · 2023-05-24T19:06:20Z

src/hotspot/share/adlc/adlparse.cpp

+  if( _curline == nullptr )       // Return null at EOF.
    return 0;


Code style.

vnkozlov · 2023-05-24T19:06:37Z

src/hotspot/share/adlc/adlparse.cpp

+  if( _curline == nullptr )       // Return null at EOF.
+    return  nullptr;


Code style.

vnkozlov · 2023-05-24T19:07:19Z

src/hotspot/share/adlc/adlparse.cpp

+  if( _curline != nullptr )         // at end of file _curchar isn't valid
    _curchar = *_ptr;               // reset _curchar to maintain invariant


Code style.

jdksjolen · 2023-05-25T12:23:27Z

Fixed! Thank you.

vnkozlov · 2023-05-25T16:04:13Z

Fixed! Thank you.

May be I was not clear. I asked to add missing {} and correct spacing: if (cond) { instead of current if( cond )

jdksjolen · 2023-05-25T17:09:18Z

/Users/runner/work/jdk/jdk/test/jdk/java/foreign/TestHFA.java:53: error: cannot find symbol
static final OfFloat FLOAT = JAVA_FLOAT.withBitAlignment(32);

Error doesn't seem relevant to this change (has to do with vector incubator).

jdksjolen · 2023-05-25T17:12:34Z

Fixed! Thank you.

May be I was not clear. I asked to add missing {} and correct spacing: if (cond) { instead of current if( cond )

Aha, I only saw the spaces issues (and not even all of those). Please have a look now.

vnkozlov

Looks good now.

jdksjolen · 2023-05-26T08:43:13Z

I'm integrating this. Only test failure on GHA seems entirely unrelated to this changeset.

/integrate

openjdk · 2023-05-26T08:44:08Z

Going to push as commit 62537d2.
Since your change was applied there have been 45 commits pushed to the master branch:

f09345b: 8308931: Problemlist compiler/jvmci/TestUncaughtErrorInCompileMethod.java
7c072db: 8308844: ProblemList gc/z/TestHighUsage.java with Generational ZGC on windows x64
4becb7b: 8306137: Open source several AWT ScrollPane related tests
199b1bf: 8308583: SIGSEGV in GraphKit::gen_checkcast
46c4da7: 8159023: Engineering notation of DecimalFormat does not work as documented
ee321c7: 8308907: ProblemList java/awt/Toolkit/GetScreenInsetsCustomGC/GetScreenInsetsCustomGC.java on linux-x64
dc7683a: 8308073: ClassLoaderExt::append_boot_classpath should handle dynamic archive
4870234: 8304375: jdk/jfr/api/consumer/filestream/TestOrdered.java failed with "Expected at least some events to be out of order! Reuse = false"
7d2a7ce: 8308672: Add version number in the replay file generated by DumpInline
27ba8bd: 8308108: Support Unicode extension for collation settings
... and 35 more: https://git.openjdk.org/jdk/compare/2d4d850813235a7533cd3bbf776adf69f90f02e6...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-26T08:44:23Z

@jdksjolen Pushed as commit 62537d2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Replace NULL with nullptr in share/adlc/

855b48b

jdksjolen marked this pull request as draft May 16, 2023 11:54

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label May 16, 2023

jdksjolen commented May 16, 2023

View reviewed changes

jdksjolen commented May 24, 2023

View reviewed changes

Fixes

7bb406f

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label May 24, 2023

jdksjolen added 4 commits May 24, 2023 19:25

Missing fail

bea2f75

Anotehr

99c147c

Last one?

713fda9

Merge remote-tracking branch 'origin/master' into JDK-8299974

46653fa

jdksjolen marked this pull request as ready for review May 24, 2023 17:54

openjdk bot added rfr Pull request is ready for review and removed merge-conflict Pull request has merge conflict with target branch labels May 24, 2023

dean-long reviewed May 24, 2023

View reviewed changes

src/hotspot/share/adlc/archDesc.cpp

@@ -85,9 +85,9 @@ void ChainList::dump() {

Copy link

Member

dean-long May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing copyright year update.

dean-long approved these changes May 24, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 24, 2023

vnkozlov reviewed May 24, 2023

View reviewed changes

Style fixes

393d84f

Fix the rest of the style issues

3b1b6d1

One more!

39cdb84

vnkozlov approved these changes May 25, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 26, 2023

openjdk bot closed this May 26, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 26, 2023

		fprintf(fp_cpp, "static const Pipeline pipeline_class_Zero_Instructions(0, 0, true, 0, 0, false, false, false, false, null, null, null, Pipeline_Use(0, 0, 0, null));\n\n");
		fprintf(fp_cpp, "static const Pipeline pipeline_class_Unknown_Instructions(0, 0, true, 0, 0, false, true, true, false, null, null, null, Pipeline_Use(0, 0, 0, null));\n\n");

		if( _curline == nullptr ) // Return null at EOF.
		return nullptr;

		if( _curline != nullptr ) // at end of file _curchar isn't valid
		_curchar = *_ptr; // reset _curchar to maintain invariant

8299974: Replace NULL with nullptr in share/adlc/ #14008

8299974: Replace NULL with nullptr in share/adlc/ #14008

Conversation

jdksjolen commented May 16, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 16, 2023

openjdk bot commented May 16, 2023

jdksjolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdksjolen commented May 16, 2023

jdksjolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented May 24, 2023

jdksjolen commented May 24, 2023

mlbridge bot commented May 24, 2023 • edited

Webrevs

Choose a reason for hiding this comment

dean-long left a comment

Choose a reason for hiding this comment

openjdk bot commented May 24, 2023 • edited

vnkozlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdksjolen commented May 25, 2023

vnkozlov commented May 25, 2023

jdksjolen commented May 25, 2023

jdksjolen commented May 25, 2023

vnkozlov left a comment

Choose a reason for hiding this comment

jdksjolen commented May 26, 2023

openjdk bot commented May 26, 2023

openjdk bot commented May 26, 2023

jdksjolen commented May 16, 2023 •

edited by openjdk bot

mlbridge bot commented May 24, 2023 •

edited

openjdk bot commented May 24, 2023 •

edited