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

[Eugene.Zelenko]

Summary

This is fix for PR26295. I added configuration option to readability-implicit-bool-cast and readability-simplify-boolean-expr.

Build and regressions were OK on RHEL6.

I think one global option will better then per-check ones, but I'm not aware of examples.

[aaron.ballman]

Missing test cases.

As for a global option, once D16113 lands, you can use getLocalOrGlobal() if you want to use a global option for this functionality.

clang-tidy/readability/ImplicitBoolCastCheck.cpp

69

Check can be a const pointer.

126

Same here.

clang-tidy/readability/ImplicitBoolCastCheck.h

32

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.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

175

Check can be a const pointer.

276

I think this also needs a storeOptions() method.

[Eugene.Zelenko]

I'll wait for global options implementation.

[LegalizeAdulthood]

clang-tidy/readability/ImplicitBoolCastCheck.h

32

This will need rebasing on the existing code, which is using "NULL" as the pre-C++11 fallback default, not "0".

36–38

I don't understand why the checks need a public getter for the nullptr literal being used.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

175

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.

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst

79–81

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.

[Eugene.Zelenko]

clang-tidy/readability/ImplicitBoolCastCheck.h

32

This was in original code. I just didn't want to change default.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

175

I had same idea, but I'm waiting for global options implementation to make other changes,

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst

79–81

English is not my native language, so help is welcomed.

[aaron.ballman]

clang-tidy/readability/ImplicitBoolCastCheck.h

32

If the code is using NULL as the pre-C++11 fallback, don't we also need an include for stddef.h/cstddef?

36–38

Agreed, good catch.

[alexfh]

Please include full context to the diffs. See http://llvm.org/docs/Phabricator.html for instructions.

clang-tidy/readability/ImplicitBoolCastCheck.h

         AllowConditionalIntegerCasts(
             Options.get("AllowConditionalIntegerCasts", 0) != 0),
         AllowConditionalPointerCasts(
-            Options.get("AllowConditionalPointerCasts", 0) != 0) {}
+            Options.get("AllowConditionalPointerCasts", 0) != 0),
+        NullPointerLiteral(
+            Options.get("NullPointerLiteral",
+            Context->getLangOpts().CPlusPlus11 ? "nullptr" : "0")) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
+  StringRef getNullPointerLiteral() const {
+    return NullPointerLiteral;
+  }
+
 private:
   void handleCastToBool(const ImplicitCastExpr *CastExpression,
                         const Stmt *ParentStatement, ASTContext &Context);

                           const ImplicitCastExpr *FurtherImplicitCastExpression,
                           ASTContext &Context);
 
-  bool AllowConditionalIntegerCasts;
-  bool AllowConditionalPointerCasts;
+  const bool AllowConditionalIntegerCasts;
+  const bool AllowConditionalPointerCasts;
+  const std::string NullPointerLiteral;
 };
 
 } // namespace tidy

clang-tidy/readability/ImplicitBoolCastCheck.cpp

 }
 
 StringRef
-getZeroLiteralToCompareWithForGivenType(CastKind CastExpressionKind,
+getZeroLiteralToCompareWithForGivenType(ImplicitBoolCastCheck *Check,
+                                        CastKind CastExpressionKind,
                                         QualType CastSubExpressionType,
                                         ASTContext &Context) {
   switch (CastExpressionKind) {

 
   case CK_PointerToBoolean:
   case CK_MemberPointerToBoolean: // Fall-through on purpose.
-    return Context.getLangOpts().CPlusPlus11 ? "nullptr" : "0";
+    return Check->getNullPointerLiteral();
 
   default:
     llvm_unreachable("Unexpected cast kind");

 }
 
 void addFixItHintsForGenericExpressionCastToBool(
-    DiagnosticBuilder &Diagnostic, const ImplicitCastExpr *CastExpression,
-    const Stmt *ParentStatement, ASTContext &Context) {
+    ImplicitBoolCastCheck *Check, DiagnosticBuilder &Diagnostic,
+    const ImplicitCastExpr *CastExpression, const Stmt *ParentStatement,
+    ASTContext &Context) {
   // In case of expressions like (! integer), we should remove the redundant not
   // operator and use inverted comparison (integer == 0).
   bool InvertComparison =

     EndLocInsertion += " != ";
   }
 
-  EndLocInsertion += getZeroLiteralToCompareWithForGivenType(
+  EndLocInsertion += getZeroLiteralToCompareWithForGivenType(Check,
       CastExpression->getCastKind(), SubExpression->getType(), Context);
 
   if (NeedOuterParens) {

     addFixItHintsForLiteralCastToBool(Diagnostic, CastExpression,
                                       EquivalentLiteralExpression);
   } else {
-    addFixItHintsForGenericExpressionCastToBool(Diagnostic, CastExpression,
-                                                ParentStatement, Context);
+    addFixItHintsForGenericExpressionCastToBool(this, Diagnostic,
+                                                CastExpression, ParentStatement,
+                                                Context);
   }
 }
 

clang-tidy/readability/SimplifyBooleanExprCheck.h

   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
+  StringRef getNullPointerLiteral() const {
+    return NullPointerLiteral;
+  }
+
 private:
   void matchBoolBinOpExpr(ast_matchers::MatchFinder *Finder, bool Value,
                           StringRef OperatorName, StringRef BooleanId);

 
   const bool ChainedConditionalReturn;
   const bool ChainedConditionalAssignment;
+  const std::string NullPointerLiteral;
 };
 
 } // namespace readability

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

   return !E->getType()->isBooleanType();
 }
 
-std::string replacementExpression(const MatchFinder::MatchResult &Result,
+std::string replacementExpression(SimplifyBooleanExprCheck *Check,
+                                  const MatchFinder::MatchResult &Result,
                                   bool Negated, const Expr *E) {
   E = E->ignoreParenBaseCasts();
   const bool NeedsStaticCast = needsStaticCast(E);

     if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) {
       if (UnOp->getOpcode() == UO_LNot) {
         if (needsNullPtrComparison(UnOp->getSubExpr()))
-          return (getText(Result, *UnOp->getSubExpr()) + " != nullptr").str();
+          return (getText(Result, *UnOp->getSubExpr()) + " != " +
+                  Check->getNullPointerLiteral()).str();
 
-        return replacementExpression(Result, false, UnOp->getSubExpr());
+        return replacementExpression(Check, Result, false, UnOp->getSubExpr());
       }
     }
 
     if (needsNullPtrComparison(E))
-      return (getText(Result, *E) + " == nullptr").str();
+      return (getText(Result, *E) + " == " +
+              Check->getNullPointerLiteral()).str();
 
     StringRef NegatedOperator;
     const Expr *LHS = nullptr;

       return ("!(" + Text + ")").str();
 
     if (needsNullPtrComparison(E))
-      return (getText(Result, *E) + " == nullptr").str();
+      return (getText(Result, *E) + " == " +
+              Check->getNullPointerLiteral()).str();
 
     return ("!" + asBool(Text, NeedsStaticCast));
   }

   if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) {
     if (UnOp->getOpcode() == UO_LNot) {
       if (needsNullPtrComparison(UnOp->getSubExpr()))
-        return (getText(Result, *UnOp->getSubExpr()) + " == nullptr").str();
+        return (getText(Result, *UnOp->getSubExpr()) + " == " +
+                Check->getNullPointerLiteral()).str();
     }
   }
 
   if (needsNullPtrComparison(E))
-    return (getText(Result, *E) + " != nullptr").str();
+    return (getText(Result, *E) + " != " +
+            Check->getNullPointerLiteral()).str();
 
   return asBool(getText(Result, *E), NeedsStaticCast);
 }

     : ClangTidyCheck(Name, Context),
       ChainedConditionalReturn(Options.get("ChainedConditionalReturn", 0U)),
       ChainedConditionalAssignment(
-          Options.get("ChainedConditionalAssignment", 0U)) {}
+          Options.get("ChainedConditionalAssignment", 0U)),
+      NullPointerLiteral(
+          Options.get("NullPointerLiteral",
+          Context->getLangOpts().CPlusPlus11 ? "nullptr" : "0")) {}
 
 void SimplifyBooleanExprCheck::matchBoolBinOpExpr(MatchFinder *Finder,
                                                   bool Value,

   const auto *LHS = Result.Nodes.getNodeAs<Expr>(LHSId);
   const auto *RHS = Result.Nodes.getNodeAs<Expr>(RHSId);
   std::string Replacement =
-      replacementExpression(Result, Negated, UseLHS ? LHS : RHS);
+      replacementExpression(this, Result, Negated, UseLHS ? LHS : RHS);
   SourceRange Range(LHS->getLocStart(), RHS->getLocEnd());
   issueDiag(Result, BoolLiteral->getLocStart(), SimplifyOperatorDiagnostic,
             Range, Replacement);

     const MatchFinder::MatchResult &Result, const ConditionalOperator *Ternary,
     bool Negated) {
   std::string Replacement =
-      replacementExpression(Result, Negated, Ternary->getCond());
+      replacementExpression(this, Result, Negated, Ternary->getCond());
   issueDiag(Result, Ternary->getTrueExpr()->getLocStart(),
             "redundant boolean literal in ternary expression result",
             Ternary->getSourceRange(), Replacement);

 void SimplifyBooleanExprCheck::replaceWithReturnCondition(
     const MatchFinder::MatchResult &Result, const IfStmt *If, bool Negated) {
   StringRef Terminator = isa<CompoundStmt>(If->getElse()) ? ";" : "";
-  std::string Condition = replacementExpression(Result, Negated, If->getCond());
+  std::string Condition =
+      replacementExpression(this, Result, Negated, If->getCond());
   std::string Replacement = ("return " + Condition + Terminator).str();
   SourceLocation Start =
       Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(ThenLiteralId)->getLocStart();

 
           const Expr *Condition = If->getCond();
           std::string Replacement =
-              "return " + replacementExpression(Result, Negated, Condition);
+              "return " + replacementExpression(this, Result, Negated,
+                                                Condition);
           issueDiag(
               Result, Lit->getLocStart(), SimplifyConditionalReturnDiagnostic,
               SourceRange(If->getLocStart(), Ret->getLocEnd()), Replacement);

       getText(Result, *Result.Nodes.getNodeAs<Expr>(IfAssignVariableId));
   StringRef Terminator = isa<CompoundStmt>(IfAssign->getElse()) ? ";" : "";
   std::string Condition =
-      replacementExpression(Result, Negated, IfAssign->getCond());
+      replacementExpression(this, Result, Negated, IfAssign->getCond());
   std::string Replacement =
       (VariableName + " = " + Condition + Terminator).str();
   SourceLocation Location =

docs/clang-tidy/checks/readability-implicit-bool-cast.rst

 
 Occurences of implicit casts inside macros and template instantiations are
 deliberately ignored, as it is not clear how to deal with such cases.
+
+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``.

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst

 ``ChainedConditionalAssignment``, respectively, is specified as non-zero.
 The default value for both options is zero.
 
+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``.