[clang-tidy] misc-unused-parameters: don't comment out parameter name for C code

[mgehre]

Summary

The fixit int square(int /*num*/) yields error: parameter name omitted for C code. Enable it only for C++ code.

[mgehre]

Fix rewrapped comment

[lebedev.ri]

Seems obviously correct, with the nit.

clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp

141–156 ↗

(On Diff #203873)

I'd recommend to instead do less confusing

// Cannot remove parameter for non-local functions.
if (Function->isExternallyVisible() ||
    !Result.SourceManager->isInMainFile(Function->getLocation()) ||
    !Indexer->getOtherRefs(Function).empty() || isOverrideMethod(Function)) {
  // It is illegal to omit parameter name here in C code, so early-out.
  if (!Result.Context->getLangOpts().CPlusPlus)
    return;

  SourceRange RemovalRange(Param->getLocation());
  // Note: We always add a space before the '/*' to not accidentally create
  // a '*/*' for pointer types, which doesn't start a comment. clang-format
  // will clean this up afterwards.
  MyDiag << FixItHint::CreateReplacement(
      RemovalRange, (Twine(" /*") + Param->getName() + "*/").str());
  return;
}

clang-tools-extra/test/clang-tidy/misc-unused-parameters.c

1–7 ↗

(On Diff #203873)

This screams a separate bug to me.
Does %check_clang_tidy not check that sources, after applying fix-it's, still parse?

[mgehre]

clang-tools-extra/test/clang-tidy/misc-unused-parameters.c

1–7 ↗

(On Diff #203873)

Unfortunately, no :-(

[mgehre]

clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp

141–156 ↗

(On Diff #203873)

Good idea, will do!

[aaron.ballman]

LGTM!

Funny enough, I've got a paper out for WG14 to try to relax this for C2x: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2381.pdf

clang-tools-extra/trunk/clang-tidy/misc/UnusedParametersCheck.cpp

void UnusedParametersCheck::warnOnUnusedParameter(
     unsigned ParamIndex) {
   const auto *Param = Function->getParamDecl(ParamIndex);
   auto MyDiag = diag(Param->getLocation(), "parameter %0 is unused") << Param;
 
   if (!Indexer) {
     Indexer = llvm::make_unique<IndexerVisitor>(*Result.Context);
   }
 
-  // Comment out parameter name for non-local functions.
+  // Cannot remove parameter for non-local functions.
   if (Function->isExternallyVisible() ||
       !Result.SourceManager->isInMainFile(Function->getLocation()) ||
       !Indexer->getOtherRefs(Function).empty() || isOverrideMethod(Function)) {
+
+    // It is illegal to omit parameter name here in C code, so early-out.
+    if (!Result.Context->getLangOpts().CPlusPlus)
+      return;
+
     SourceRange RemovalRange(Param->getLocation());
-    // Note: We always add a space before the '/*' to not accidentally create a
-    // '*/*' for pointer types, which doesn't start a comment. clang-format will
-    // clean this up afterwards.
+    // Note: We always add a space before the '/*' to not accidentally create
+    // a '*/*' for pointer types, which doesn't start a comment. clang-format
+    // will clean this up afterwards.
     MyDiag << FixItHint::CreateReplacement(
         RemovalRange, (Twine(" /*") + Param->getName() + "*/").str());
     return;
   }
 
   // Fix all redeclarations.
   for (const FunctionDecl *FD : Function->redecls())
     if (FD->param_size())

clang-tools-extra/trunk/test/clang-tidy/misc-unused-parameters.c

 // RUN: %check_clang_tidy %s misc-unused-parameters %t -- -- -xc
 
 // Basic removal
 // =============
 void a(int i) {;}
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: parameter 'i' is unused [misc-unused-parameters]
-// CHECK-FIXES: {{^}}void a(int  /*i*/) {;}{{$}}
+// CHECK-FIXES: {{^}}void a(int i) {;}{{$}}
 
 static void b(); // In C, forward declarations can leave out parameters.
 static void b(int i) {;}
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: parameter 'i' is unused [misc-unused-parameters]
 // CHECK-FIXES: {{^}}static void b() {;}{{$}}
 
 // Unchanged cases
 // ===============
 void h(i, c, d) int i; char *c, *d; {} // Don't mess with K&R style