[clang-tidy] Refactoring of FixHintUtils

[etienneb]

Summary

Refactor some checkers to use the tooling re-writing API.
see: http://reviews.llvm.org/D19941

[alexfh]

clang-tidy/misc/SwappedArgumentsCheck.cpp

51–52

Stray backslash at EOL.

clang-tidy/utils/FixItHintUtils.cpp

40

Please update the comment.

clang-tidy/utils/FixItHintUtils.h

21

These functions are not clang-tidy specific. They might be convenient in clang/include/clang/Tooling. Manuel, WDYT?

38

nit: functionsShouldStartWithALowerCase in LLVM.

38

Please document the functions. Specifically, we need to note that they can return a FixItHint that isNull in case the range is not a simple file range.

54–55

Please update the comment.

[etienneb]

I'm mostly interested to validate the concept before pushing further the refactoring.

The same API is marked "deprecated" internally.
I just want to confirm I'm not missing an elephant in the room.

alexfh seems willing to push this as-is (modulo cleanup, packaging and tests)...

clang-tidy/misc/SwappedArgumentsCheck.cpp

51–52

yeah, a few nits.
I didn't format too. :)

clang-tidy/utils/FixItHintUtils.h

21

I didn't think about that yet?
Should we do this in two steps!?
First, lift them in clang-tidy to see the use cases.
Then, generalized them if possible and if it worth it?

I do not know enough on other tooling, yet!

54–55

I'm preparing a separate CL to rename these namespace too.
Forget about them in this patch.

[etienneb]

step

[etienneb]

reformat

[klimek]

clang-tidy/utils/FixItHintUtils.h

21

If I'm not mistaken, this is only for template-cases to work, which are used in this file. I don't think this function carries its weight for a more general solution. I expect everybody else to just write SourceRange(Loc) - I don't think it's any less obvious than getSourceRange(Loc).

[alexfh]

clang-tidy/utils/FixItHintUtils.h

21

Yes, getSourceRange is needed for the the template getText function below. On its own it doesn't seem to be particularly useful and should probably be moved to the internal namespace.

The useful API here that makes sense to be moved to clang/Tooling (and that I was talking about) is the getText function and maybe also createRemoval and createReplacement.

Do you think they fit well to clang/Tooling? If so, what specific file would be the best place for them?

test/clang-tidy/misc-swapped-arguments.cpp

26

Please leave the CHECK-FIXES line to actually verify that this line is not changed.

[klimek]

clang-tidy/utils/FixItHintUtils.h

21

Ok, after short in-person chat I now understand what this is about:
we don't have good abstractions for ast-node based source abstractions (Replacements has this to some degree, but intentionally distances itself from the AST to be able to live in a small 'core' library).
Those might be a good idea to have, and should probably live in Tooling/AST.h or Tooling/ASTNodes.h or somesuch. Generally, Alex has convinced me he has a good design idea there, so I'll let him take it from here :)

[etienneb]

clang-tidy/utils/FixItHintUtils.h

38

This is really inconsistent accross the codebase! :(

return FixItHint::CreateRemoval(getSourceRange(Node));
                  ^^^^^^^^^^^^^^

But, I'm not against to follow any rules, in this case, the one on the website.

54–55

separate CL on-going.

[etienneb]

test/clang-tidy/misc-swapped-arguments.cpp

26

This line was correctly rewrtited.
Turn out there is more logic under the GetText.
I need to write more use cases to validate if it's always doing the right thing.

[etienneb]

split the work

[etienneb]

add unittests

[alexfh]

LG. Thank you for cleaning this up!

clang-tidy/misc/SwappedArgumentsCheck.cpp

 //===--- SwappedArgumentsCheck.cpp - clang-tidy ---------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "SwappedArgumentsCheck.h"
+#include "../utils/FixItHintUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/SmallPtrSet.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
return Cast->getCastKind() == CK_UserDefinedConversion ||
          Cast->getCastKind() == CK_FloatingToBoolean ||
          Cast->getCastKind() == CK_FloatingToIntegral ||
          Cast->getCastKind() == CK_IntegralToBoolean ||
          Cast->getCastKind() == CK_IntegralToFloating ||
          Cast->getCastKind() == CK_MemberPointerToBoolean ||
          Cast->getCastKind() == CK_PointerToBoolean;
 }
 
-/// \brief Get a StringRef representing a SourceRange.
-static StringRef getAsString(const MatchFinder::MatchResult &Result,
-                             SourceRange R) {
-  const SourceManager &SM = *Result.SourceManager;
-  // Don't even try to resolve macro or include contraptions. Not worth emitting
-  // a fixit for.
-  if (R.getBegin().isMacroID() ||
-      !SM.isWrittenInSameFile(R.getBegin(), R.getEnd()))
-    return StringRef();
-
-  const char *Begin = SM.getCharacterData(R.getBegin());
-  const char *End = SM.getCharacterData(Lexer::getLocForEndOfToken(
-      R.getEnd(), 0, SM, Result.Context->getLangOpts()));
-
-  return StringRef(Begin, End - Begin);
-}
-
 void SwappedArgumentsCheck::check(const MatchFinder::MatchResult &Result) {
-  auto *Call = Result.Nodes.getStmtAs<CallExpr>("call");
+  const ASTContext &Ctx = *Result.Context;
+  const auto *Call = Result.Nodes.getStmtAs<CallExpr>("call");
 
   llvm::SmallPtrSet<const Expr *, 4> UsedArgs;
   for (unsigned I = 1, E = Call->getNumArgs(); I < E; ++I) {
     const Expr *LHS = Call->getArg(I - 1);
     const Expr *RHS = Call->getArg(I);
 
     // Only need to check RHS, as LHS has already been covered. We don't want to
     // emit two warnings for a single argument.
for (unsigned I = 1, E = Call->getNumArgs(); I < E; ++I) {
     const Expr *LHSFrom = ignoreNoOpCasts(LHSCast->getSubExpr());
     const Expr *RHSFrom = ignoreNoOpCasts(RHSCast->getSubExpr());
     if (LHS->getType() == RHS->getType() ||
         LHS->getType() != RHSFrom->getType() ||
         RHS->getType() != LHSFrom->getType())
       continue;
 
     // Emit a warning and fix-its that swap the arguments.
-    SourceRange LHSRange = LHS->getSourceRange(),
-                RHSRange = RHS->getSourceRange();
-    auto D =
-        diag(Call->getLocStart(), "argument with implicit conversion from %0 "
-                                  "to %1 followed by argument converted from "
-                                  "%2 to %3, potentially swapped arguments.")
         << LHS->getType() << LHSFrom->getType() << RHS->getType()
-        << RHSFrom->getType() << LHSRange << RHSRange;
-
-    StringRef RHSString = getAsString(Result, RHSRange);
-    StringRef LHSString = getAsString(Result, LHSRange);
-    if (!LHSString.empty() && !RHSString.empty()) {
-      D << FixItHint::CreateReplacement(
-               CharSourceRange::getTokenRange(LHSRange), RHSString)
-        << FixItHint::CreateReplacement(
-               CharSourceRange::getTokenRange(RHSRange), LHSString);
-    }
+        << RHSFrom->getType()
+        << utils::fixit::createReplacement(*LHS, *RHS, Ctx)
+        << utils::fixit::createReplacement(*RHS, *LHS, Ctx);
 
     // Remember that we emitted a warning for this argument.
     UsedArgs.insert(RHSCast);
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

clang-tidy/performance/ForRangeCopyCheck.cpp

bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl &LoopVar,
   llvm::Optional<bool> Expensive =
       type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
   if (!Expensive || !*Expensive)
     return false;
   auto Diagnostic =
       diag(LoopVar.getLocation(),
            "the loop variable's type is not a reference type; this creates a "
            "copy in each iteration; consider making this a reference")
-      << utils::create_fix_it::changeVarDeclToReference(LoopVar, Context);
+      << utils::fixit::changeVarDeclToReference(LoopVar, Context);
   if (!LoopVar.getType().isConstQualified())
-    Diagnostic << utils::create_fix_it::changeVarDeclToConst(LoopVar);
+    Diagnostic << utils::fixit::changeVarDeclToConst(LoopVar);
   return true;
 }
 
 bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
     const VarDecl &LoopVar, const CXXForRangeStmt &ForRange,
     ASTContext &Context) {
   llvm::Optional<bool> Expensive =
       type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
   if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
     return false;
   if (!decl_ref_expr_utils::isOnlyUsedAsConst(LoopVar, *ForRange.getBody(), Context))
     return false;
   diag(LoopVar.getLocation(),
        "loop variable is copied but only used as const reference; consider "
        "making it a const reference")
-      << utils::create_fix_it::changeVarDeclToConst(LoopVar)
-      << utils::create_fix_it::changeVarDeclToReference(LoopVar, Context);
+      << utils::fixit::changeVarDeclToConst(LoopVar)
+      << utils::fixit::changeVarDeclToReference(LoopVar, Context);
   return true;
 }
 
 } // namespace performance
 } // namespace tidy
 } // namespace clang

clang-tidy/performance/UnnecessaryCopyInitialization.cpp

 namespace {
 
 void recordFixes(const VarDecl &Var, ASTContext &Context,
                  DiagnosticBuilder &Diagnostic) {
   // Do not propose fixes in macros since we cannot place them correctly.
   if (Var.getLocation().isMacroID())
     return;
 
-  Diagnostic << utils::create_fix_it::changeVarDeclToReference(Var, Context);
+  Diagnostic << utils::fixit::changeVarDeclToReference(Var, Context);
   if (!Var.getType().isLocalConstQualified())
-    Diagnostic << utils::create_fix_it::changeVarDeclToConst(Var);
+    Diagnostic << utils::fixit::changeVarDeclToConst(Var);
 }
 
 } // namespace
 
 
 using namespace ::clang::ast_matchers;
 using decl_ref_expr_utils::isOnlyUsedAsConst;
 

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

auto Diag =
                               "consider making it a const reference")
       << paramNameOrIndex(Param->getName(), Index);
   // Do not propose fixes in macros since we cannot place them correctly.
   if (Param->getLocStart().isMacroID())
     return;
   for (const auto *FunctionDecl = Function; FunctionDecl != nullptr;
        FunctionDecl = FunctionDecl->getPreviousDecl()) {
     const auto &CurrentParam = *FunctionDecl->getParamDecl(Index);
-    Diag << utils::create_fix_it::changeVarDeclToReference(CurrentParam,
-                                                           *Result.Context);
+    Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
     if (!IsConstQualified)
-      Diag << utils::create_fix_it::changeVarDeclToConst(CurrentParam);
+      Diag << utils::fixit::changeVarDeclToConst(CurrentParam);
   }
 }
 
 } // namespace performance
 } // namespace tidy
 } // namespace clang

clang-tidy/readability/ElseAfterReturnCheck.cpp

 //===--- ElseAfterReturnCheck.cpp - clang-tidy-----------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ElseAfterReturnCheck.h"
+#include "../utils/FixItHintUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
   // FIXME: Support continue, break and throw.
   Finder->addMatcher(
       compoundStmt(
           forEach(ifStmt(hasThen(stmt(anyOf(returnStmt(),
                                             compoundStmt(has(returnStmt()))))),
                          hasElse(stmt().bind("else")))
                       .bind("if"))),
       this);
 }
 
-static FixItHint removeToken(SourceLocation Loc) {
-  return FixItHint::CreateRemoval(CharSourceRange::getTokenRange(Loc, Loc));
-}
-
 void ElseAfterReturnCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *If = Result.Nodes.getNodeAs<IfStmt>("if");
   SourceLocation ElseLoc = If->getElseLoc();
   DiagnosticBuilder Diag = diag(ElseLoc, "don't use else after return");
-  Diag << removeToken(ElseLoc);
+  Diag << utils::fixit::createRemoval(ElseLoc);
 
   // FIXME: Removing the braces isn't always safe. Do a more careful analysis.
   // FIXME: Change clang-format to correctly un-indent the code.
   if (const auto *CS = Result.Nodes.getNodeAs<CompoundStmt>("else"))
-    Diag << removeToken(CS->getLBracLoc()) << removeToken(CS->getRBracLoc());
+    Diag << utils::fixit::createRemoval(CS->getLBracLoc())
+         << utils::fixit::createRemoval(CS->getRBracLoc());
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tidy/utils/FixItHintUtils.h

 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_FIXITHINTUTILS_H
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
-namespace create_fix_it {
+namespace fixit {
+
+inline SourceRange getSourceRange(const SourceLocation &Loc) {
+  return SourceRange(Loc);
+}
+
+inline SourceRange getSourceRange(const SourceRange &Range) { return Range; }
+
+template <typename T> SourceRange getSourceRange(const T &Node) {
+  return Node.getSourceRange();
+}
+
+StringRef getText(SourceRange Range, const ASTContext &Context);
+
+template <typename T>
+StringRef getText(const T &Node, const ASTContext &Context) {
+  return getText(getSourceRange(Node), Context);
+}
+
+template <typename T> FixItHint createRemoval(const T &Node) {
+  return FixItHint::CreateRemoval(getSourceRange(Node));
+}
+
+template <typename D, typename S>
+FixItHint createReplacement(const D &Destination, const S &Source,
+                            const ASTContext &Context) {
+  return FixItHint::CreateReplacement(getSourceRange(Destination),
+                                      getText(Source, Context));
+}
 
 /// \brief Creates fix to make VarDecl a reference by adding '&'.
 FixItHint changeVarDeclToReference(const VarDecl &Var, ASTContext &Context);
 
 /// \brief Creates fix to make VarDecl const qualified.
 FixItHint changeVarDeclToConst(const VarDecl &Var);
 
-} // namespace create_fix_it
-} // utils
+} // namespace fixit
+} // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_FIXITHINTUTILS_H

clang-tidy/utils/FixItHintUtils.cpp

 //===--- FixItHintUtils.cpp - clang-tidy-----------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "FixItHintUtils.h"
 #include "LexerUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/Lex/Lexer.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
-namespace create_fix_it {
+namespace fixit {
+
+StringRef getText(SourceRange Range, const ASTContext &Context) {
+  return Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
+                              Context.getSourceManager(),
+                              Context.getLangOpts());
+}
 
 FixItHint changeVarDeclToReference(const VarDecl &Var, ASTContext &Context) {
   SourceLocation AmpLocation = Var.getLocation();
   auto Token = lexer_utils::getPreviousNonCommentToken(Context, AmpLocation);
   if (!Token.is(tok::unknown))
     AmpLocation = Lexer::getLocForEndOfToken(Token.getLocation(), 0,
                                              Context.getSourceManager(),
                                              Context.getLangOpts());
   return FixItHint::CreateInsertion(AmpLocation, "&");
 }
 
 FixItHint changeVarDeclToConst(const VarDecl &Var) {
   return FixItHint::CreateInsertion(Var.getTypeSpecStartLoc(), "const ");
 }
 
 } // namespace create_fix_it
 } // namespace utils
 } // namespace tidy
 } // namespace clang

test/clang-tidy/misc-swapped-arguments.cpp

 // CHECK-FIXES: F(3, 1.0)
 
 #define M(x, y) x##y()
 
   double b = 1.0;
   F(b, M(Some, Function));
 // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments.
 // In macro, don't emit fixits.
-// CHECK-FIXES: F(b, M(Some, Function));
 
 #define N F(b, SomeFunction())
 
   N;
 // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments.
 // In macro, don't emit fixits.
 // CHECK-FIXES: #define N F(b, SomeFunction())