[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Sorry about the delayed review on this -- I had intended to provide comments earlier, but somehow this fell off my radar.



================
Comment at: docs/ReleaseNotes.rst:53
 
+- ``-Wextra-semi-stmt`` is a new diagnostic, which is, much like
+  ``-Wextra-semi``, diagnoses extra semicolons. This new diagnostic
----------------
, which is, much like `-Wextra-semi`, diagnoses extra semicolons. -> that diagnoses extra semicolons, much like `-Wextra-semi`.


================
Comment at: docs/ReleaseNotes.rst:55
+  ``-Wextra-semi``, diagnoses extra semicolons. This new diagnostic
+  fires on all *unnecessary* null statements (expression statements without
+  an expression), unless: the semi directly follows a macro that was
----------------
fires on all -> diagnoses


================
Comment at: docs/ReleaseNotes.rst:56
+  fires on all *unnecessary* null statements (expression statements without
+  an expression), unless: the semi directly follows a macro that was
+  expanded to nothing; the semi is within the macro itself (both macros from
----------------
semi -> semicolon


================
Comment at: docs/ReleaseNotes.rst:57
+  an expression), unless: the semi directly follows a macro that was
+  expanded to nothing; the semi is within the macro itself (both macros from
+  system headers, and normal macros).
----------------
nothing; the semi is -> nothing or if the semicolon

I'd make the parenthetical a stand-alone sentence. Something like: "This applies to macros defined in system headers as well as user-defined macros."


================
Comment at: docs/ReleaseNotes.rst:65-68
+      struct S {
+        int *begin();
+        int *end();
+      };
----------------
I'd drop this from the example.


================
Comment at: docs/ReleaseNotes.rst:91
+
+- ``-Wempty-init-stmt`` is a new diagnostic, which is diagnoses empty
+  init-statements of ``if``, ``switch``, ``range-based for``, unless: the semi
----------------
diagnostic, which is diagnoses empty -> diagnostic that diagnoses empty


================
Comment at: docs/ReleaseNotes.rst:92
+- ``-Wempty-init-stmt`` is a new diagnostic, which is diagnoses empty
+  init-statements of ``if``, ``switch``, ``range-based for``, unless: the semi
+  directly follows a macro that was expanded to nothing; the semi is within the
----------------
semi -> semicolon


================
Comment at: docs/ReleaseNotes.rst:93-94
+  init-statements of ``if``, ``switch``, ``range-based for``, unless: the semi
+  directly follows a macro that was expanded to nothing; the semi is within the
+  macro itself (both macros from system headers, and normal macros).
+  This diagnostic in ``-Wextra-semi-stmt`` group, and is enabled in ``-Wextra``.
----------------
nothing; the semi is -> nothing or if the semicolon

I'd make the parenthetical a stand-alone sentence. Something like: "This applies to macros defined in system headers as well as user-defined macros."


================
Comment at: docs/ReleaseNotes.rst:95
+  macro itself (both macros from system headers, and normal macros).
+  This diagnostic in ``-Wextra-semi-stmt`` group, and is enabled in ``-Wextra``.
+
----------------
diagnostic in -> diagnostic is in the

Drop the comma.


================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:57-58
+def warn_null_statement : Warning<
+  "statement has no effect, ';' "
+  "with no preceding expression is a null statement">,
+  InGroup<ExtraSemiStmt>, DefaultIgnore;
----------------
How about: `empty expression statement has no effect; remove unnecessary ';' to silence this warning` ?


================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:557-558
+def warn_empty_init_statement : Warning<
+  "init-statement of '%select{if|switch|range-based for}0' is a "
+  "null statement">, InGroup<EmptyInitStatement>, DefaultIgnore;
 
----------------
How about: `empty initialization statement of '%select{if|switch|range-based for}0' has no effect`?


================
Comment at: test/Parser/extra-semi-resulting-in-nullstmt.cpp:81
+  }
+}
----------------
There are four additional tests I'd like to see: 1) a semicolon at global scope, 2) a semicolon after a namespace declaration (e.g., `namespace foo {};`), 3) a `[[fallthrough]];` null statement, 4) a `[[]];` null statement.


Repository:
  rC Clang

https://reviews.llvm.org/D52695