Fix bug in suggested fix that truncated variable names to 1 character.

[sbenza]

Summary

Fix bug in suggested fix that truncated variable names to 1 character.
Also, rework the suggested fix to try to remove unnecessary whitespace.

[aaron.ballman]

LGTM!

clang-tidy/misc/UnusedParametersCheck.cpp

25

Are we preferring anonymous namespaces over static functions these days?

[sbenza]

clang-tidy/misc/UnusedParametersCheck.cpp

25

I didn't know there was a preference.
I usually do anon namespaces, but these functions were already here as static.

[aaron.ballman]

clang-tidy/misc/UnusedParametersCheck.cpp

25

There is, and I just went to find it instead of asking you to do it for me. ;-) http://llvm.org/docs/CodingStandards.html#anonymous-namespaces

Since this code doesn't involve any class declarations, static is totally fine.

clang-tidy/misc/UnusedParametersCheck.cpp

 //===--- UnusedParametersCheck.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 "UnusedParametersCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 
 void UnusedParametersCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(functionDecl().bind("function"), this);
 }
 
-static FixItHint removeParameter(const FunctionDecl *Function, unsigned Index) {
-  const ParmVarDecl *Param = Function->getParamDecl(Index);
-  unsigned ParamCount = Function->getNumParams();
-  SourceRange RemovalRange = Param->getSourceRange();
-  if (ParamCount == 1)
-    return FixItHint::CreateRemoval(RemovalRange);
-
-  if (Index == 0)
-    RemovalRange.setEnd(
-        Function->getParamDecl(Index + 1)->getLocStart().getLocWithOffset(-1));
-  else
-    RemovalRange.setBegin(
-        Function->getParamDecl(Index - 1)->getLocEnd().getLocWithOffset(1));
-
-  return FixItHint::CreateRemoval(RemovalRange);
-}
-
-static FixItHint removeArgument(const CallExpr *Call, unsigned Index) {
-  unsigned ArgCount = Call->getNumArgs();
-  const Expr *Arg = Call->getArg(Index);
-  SourceRange RemovalRange = Arg->getSourceRange();
-  if (ArgCount == 1)
-    return FixItHint::CreateRemoval(RemovalRange);
-  if (Index == 0)
-    RemovalRange.setEnd(
-        Call->getArg(Index + 1)->getLocStart().getLocWithOffset(-1));
-  else
-    RemovalRange.setBegin(
-        Call->getArg(Index - 1)->getLocEnd().getLocWithOffset(1));
-  return FixItHint::CreateRemoval(RemovalRange);
+template <typename T>
+static CharSourceRange removeNode(const MatchFinder::MatchResult &Result,
+                                  const T *PrevNode, const T *Node,
+                                  const T *NextNode) {
+  if (NextNode)
+    return CharSourceRange::getCharRange(Node->getLocStart(),
+                                         NextNode->getLocStart());
+
+  if (PrevNode)
+    return CharSourceRange::getTokenRange(
+        Lexer::getLocForEndOfToken(PrevNode->getLocEnd(), 0,
+                                   *Result.SourceManager,
+                                   Result.Context->getLangOpts()),
+        Node->getLocEnd());
+
+  return CharSourceRange::getTokenRange(Node->getSourceRange());
+}
+
+static FixItHint removeParameter(const MatchFinder::MatchResult &Result,
+                                 const FunctionDecl *Function, unsigned Index) {
+  return FixItHint::CreateRemoval(removeNode(
+      Result, Index > 0 ? Function->getParamDecl(Index - 1) : nullptr,
+      Function->getParamDecl(Index),
+      Index + 1 < Function->getNumParams() ? Function->getParamDecl(Index + 1)
+                                           : nullptr));
+}
+
+static FixItHint removeArgument(const MatchFinder::MatchResult &Result,
+                                const CallExpr *Call, unsigned Index) {
+  return FixItHint::CreateRemoval(removeNode(
+      Result, Index > 0 ? Call->getArg(Index - 1) : nullptr,
+      Call->getArg(Index),
+      Index + 1 < Call->getNumArgs() ? Call->getArg(Index + 1) : nullptr));
 }
 
 void UnusedParametersCheck::warnOnUnusedParameter(
     const MatchFinder::MatchResult &Result, const FunctionDecl *Function,
     unsigned ParamIndex) {
   const auto *Param = Function->getParamDecl(ParamIndex);
   auto MyDiag = diag(Param->getLocation(), "parameter '%0' is unused")
                 << Param->getName();
if (Function->isExternallyVisible() ||
     MyDiag << FixItHint::CreateReplacement(
         RemovalRange, (Twine(" /*") + Param->getName() + "*/").str());
     return;
   }
 
   // Fix all redeclarations.
   for (const FunctionDecl *FD : Function->redecls())
     if (FD->param_size())
-      MyDiag << removeParameter(FD, ParamIndex);
+      MyDiag << removeParameter(Result, FD, ParamIndex);
 
   // Fix all call sites.
   auto CallMatches = ast_matchers::match(
       decl(forEachDescendant(
           callExpr(callee(functionDecl(equalsNode(Function)))).bind("x"))),
       *Result.Context->getTranslationUnitDecl(), *Result.Context);
   for (const auto &Match : CallMatches)
-    MyDiag << removeArgument(Match.getNodeAs<CallExpr>("x"), ParamIndex);
+    MyDiag << removeArgument(Result, Match.getNodeAs<CallExpr>("x"),
+                             ParamIndex);
 }
 
 void UnusedParametersCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("function");
   if (!Function->doesThisDeclarationHaveABody() ||
       !Function->hasWrittenPrototype())
     return;
   if (const auto *Method = dyn_cast<CXXMethodDecl>(Function))

test/clang-tidy/misc-unused-parameters.cpp

 // CHECK-FIXES: {{^}}static void staticFunctionA()
 
 static void staticFunctionB(int i, int j) { (void)i; }
 // CHECK-MESSAGES: :[[@LINE-1]]:40: warning
 // CHECK-FIXES: {{^}}static void staticFunctionB(int i)
 
 static void staticFunctionC(int i, int j) { (void)j; }
 // CHECK-MESSAGES: :[[@LINE-1]]:33: warning
-// CHECK-FIXES: {{^}}static void staticFunctionC( int j)
+// CHECK-FIXES: {{^}}static void staticFunctionC(int j)
 
 static void staticFunctionD(int i, int j, int k) { (void)i; (void)k; }
 // CHECK-MESSAGES: :[[@LINE-1]]:40: warning
 // CHECK-FIXES: {{^}}static void staticFunctionD(int i, int k)
 
 static void staticFunctionE(int i = 4) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:33: warning
 // CHECK-FIXES: {{^}}static void staticFunctionE()
 
 
 static void someCallSites() {
   staticFunctionA(1);
 // CHECK-FIXES: staticFunctionA();
   staticFunctionB(1, 2);
 // CHECK-FIXES: staticFunctionB(1);
   staticFunctionC(1, 2);
-// CHECK-FIXES: staticFunctionC( 2);
+// CHECK-FIXES: staticFunctionC(2);
   staticFunctionD(1, 2, 3);
 // CHECK-FIXES: staticFunctionD(1, 3);
   staticFunctionE();
 }
 
+/*
+ * FIXME: This fails because the removals overlap and ClangTidy doesn't apply
+ *        them.
+ * static void bothVarsUnused(int a, int b) {}
+ */
+
+// Regression test for long variable names and expressions
+// =======================================================
+static int variableWithLongName1(int LongName1, int LongName2) {
+// CHECK-MESSAGES: :[[@LINE-1]]:53: warning: parameter 'LongName2' is unused
+// CHECK-FIXES: {{^}}static int variableWithLongName1(int LongName1) {
+  return LongName1;
+}
+static int variableWithLongName2(int LongName1, int LongName2) {
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: parameter 'LongName1' is unused
+// CHECK-FIXES: {{^}}static int variableWithLongName2(int LongName2) {
+  return LongName2;
+}
+static void someLongNameCallSites() {
+  int LongName1 = 7, LongName2 = 17;
+  variableWithLongName1(LongName1, LongName2);
+// CHECK-FIXES: variableWithLongName1(LongName1);
+  variableWithLongName2(LongName1, LongName2);
+// CHECK-FIXES: variableWithLongName2(LongName2);
+}
+
 class SomeClass {
   static void f(int i) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:21: warning
 // CHECK-FIXES: static void f(int  /*i*/) {}
 };
 
 namespace {
 class C {