[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

41 ↗

(On Diff #55822)

Please update the comment.

clang-tidy/utils/FixItHintUtils.h

21 ↗

(On Diff #55822)

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

38 ↗

(On Diff #55822)

nit: functionsShouldStartWithALowerCase in LLVM.

38 ↗

(On Diff #55822)

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.

56 ↗

(On Diff #55822)

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 ↗

(On Diff #55822)

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!

56 ↗

(On Diff #55822)

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 ↗

(On Diff #55893)

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 ↗

(On Diff #55893)

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 ↗

(On Diff #55893)

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

[klimek]

clang-tidy/utils/FixItHintUtils.h

21 ↗

(On Diff #55893)

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 ↗

(On Diff #55822)

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.

56 ↗

(On Diff #55822)

separate CL on-going.

[etienneb]

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

26 ↗

(On Diff #55893)

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 "clang/AST/ASTContext.h"
 #include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Fixit.h"
 #include "llvm/ADT/SmallPtrSet.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
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()
+        << tooling::fixit::createReplacement(*LHS, *RHS, Ctx)
+        << tooling::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/readability/ElseAfterReturnCheck.cpp

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);
 
   // 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 << removeToken(CS->getLBracLoc())
+         << removeToken(CS->getRBracLoc());
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang