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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
