[PATCH] D12081: Add use-nullptr check to clang-tidy.

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Sorry for the delay. Here are some more comments.


================
Comment at: clang-tidy/modernize/ModernizeTidyModule.cpp:33
@@ -30,2 +32,3 @@
     Opts["modernize-pass-by-value.IncludeStyle"] = "llvm"; // Also: "google".
+    Opts["modernize-use-nullptr.UserNullMacros"] = "reasonable";
     return Options;
----------------
"reasonable"? ;)

================
Comment at: clang-tidy/modernize/UseNullptrCheck.cpp:80
@@ +79,3 @@
+  if (isAlphanumeric(*FullSourceLoc(PreviousLocation, SM).getCharacterData())) {
+    Check.diag(Range.getBegin(), "use nullptr")
+        << FixItHint::CreateReplacement(Range, " nullptr");
----------------
The two statements differ only in the replacement string. I'd write this instead:
  bool NeedsSpace = isAlphanumeric(*FullSourceLoc(PreviousLocation, SM).getCharacterData());
  Check.diag(...) << FixItHint::CreateReplacement(Range, NeedsSpace ? " nullptr" : "nullptr");


================
Comment at: clang-tidy/modernize/UseNullptrCheck.cpp:250
@@ +249,3 @@
+      return skipSubTree();
+    } // If NullTo(Member)Pointer cast.
+
----------------
Please reverse the condition and use an early return.

================
Comment at: clang-tidy/modernize/UseNullptrCheck.cpp:292
@@ +291,3 @@
+
+    if (ArgUsageVisitor.foundInvalid())
+      return false;
----------------
nit: `return !ArgUsageVisitor.foundInvalid();`

================
Comment at: clang-tidy/modernize/UseNullptrCheck.cpp:313
@@ +312,3 @@
+    // Find the location of the immediate macro expansion.
+    while (1) {
+      std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(ArgLoc);
----------------
`while (true)`

================
Comment at: clang-tidy/modernize/UseNullptrCheck.cpp:376
@@ +375,3 @@
+        if (Loc.isFileID()) {
+          if (Loc == TestMacroLoc) {
+            // Match made.
----------------
Replace this with `return Loc == TestMacroLoc;`.

================
Comment at: clang-tidy/modernize/UseNullptrCheck.cpp:390
@@ +389,3 @@
+      MacroLoc = SM.getImmediateExpansionRange(Loc).first;
+      if (MacroLoc.isFileID() && MacroLoc == TestMacroLoc)
+        // Match made.
----------------
nit: Please add braces.

================
Comment at: clang-tidy/modernize/UseNullptrCheck.cpp:394
@@ +393,3 @@
+
+      Loc = Expansion.getSpellingLoc();
+      Loc = Expansion.getSpellingLoc().getLocWithOffset(LocInfo.second);
----------------
wat

================
Comment at: clang-tidy/modernize/UseNullptrCheck.cpp:396
@@ +395,3 @@
+      Loc = Expansion.getSpellingLoc().getLocWithOffset(LocInfo.second);
+      if (Loc.isFileID())
+        // If we made it this far without finding a match, there is no match to
----------------
nit: Please add braces.

================
Comment at: clang-tidy/modernize/UseNullptrCheck.cpp:442
@@ +441,3 @@
+      Start = Parent;
+    } while (1);
+
----------------
`while (true)`

================
Comment at: test/clang-tidy/modernize-use-nullptr-basic.cpp:2
@@ +1,3 @@
+// RUN: $(dirname %s)/check_clang_tidy.sh %s modernize-use-nullptr %t -- \
+// -std=c++98 -Wno-non-literal-null-conversion
+// REQUIRES: shell
----------------
Please either merge these lines or add `RUN:   ` (with three spaces to make the continuation line indented) before `-std...`.

================
Comment at: test/clang-tidy/modernize-use-nullptr-basic.cpp:328
@@ +327,3 @@
+
+// FIXME
+template<typename T>
----------------
Please expand the comment explaining what this FIXME is about.

================
Comment at: test/clang-tidy/modernize-use-nullptr.cpp:1
@@ +1,2 @@
+// RUN:   $(dirname %s)/check_clang_tidy.sh %s modernize-use-nullptr %t \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nullptr.UserNullMacros, value: 'MY_NULL'}]}" \
----------------
The first line should not be indented. The purpose of indentation here is to show that the other lines relate to the same command. Sorry if I was unclear.

================
Comment at: test/clang-tidy/modernize-use-nullptr.cpp:4
@@ +3,3 @@
+// RUN:   -- -std=c++11
+
+// REQUIRES: shell
----------------
nit: Please remove the empty line.


http://reviews.llvm.org/D12081