[PATCH] D16700: [Clang-tidy] Make null pointer literals for fixes configurable for two checks

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.

================
Comment at: clang-tidy/readability/ImplicitBoolCastCheck.h:32
@@ -30,1 +31,3 @@
+            Options.get("NullPointerLiteral",
+            Context->getLangOpts().CPlusPlus11 ? "nullptr" : "0")) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
----------------
aaron.ballman wrote:
> I know you are following the pattern used in the code here, but I think this class needs a storeOptions() function as well. See AssertSideEffectCheck as an example.
This will need rebasing on the existing code, which is using "NULL" as the pre-C++11 fallback default, not "0".

================
Comment at: clang-tidy/readability/ImplicitBoolCastCheck.h:36-38
@@ -32,1 +35,5 @@
 
+  StringRef getNullPointerLiteral() const {
+    return NullPointerLiteral;
+  }
+
----------------
I don't understand why the checks need a public getter for the nullptr literal being used.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:175
@@ -174,2 +174,3 @@
 
-std::string replacementExpression(const MatchFinder::MatchResult &Result,
+std::string replacementExpression(SimplifyBooleanExprCheck *Check,
+                                  const MatchFinder::MatchResult &Result,
----------------
aaron.ballman wrote:
> Check can be a const pointer.
Passing the check in here is overkill.  This helper function isn't going to ever be used for multiple checks and the only thing you ever do with the check is get the nullptr literal, so just pass in the thing it needs directly.

This will also need to be rebased onto the current code.

================
Comment at: docs/clang-tidy/checks/readability-simplify-boolean-expr.rst:79-81
@@ -78,1 +78,4 @@
 
+Null pointer literal for fixes could be changed with option
+``NullPointerLiteral``. The default value for C++11 or later is ``nullptr``, for
+ C++98/C++03 - ``0``.
----------------
The wording here is awkward.  Instead, I suggest:

```
The option ``NullPointerLiteral`` configures the text used for comparisons of pointers
against zero to synthesize boolean expressions.  The default value for C++11 or later
is ``nullptr``, otherwise the value is ``NULL``.
```

It's subjective, but my experience is that pre C++11 code bases prefer `NULL` over `0`.


Repository:
  rL LLVM

http://reviews.llvm.org/D16700