[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:35-38
+    SourceRange RemovalRange(
+          Lexer::getLocForEndOfToken(D->getLocation(), 0, 
+                *Result.SourceManager, Result.Context->getLangOpts()),
+          D->getDefaultArgRange().getEnd()
----------------
juliehockett wrote:
> aaron.ballman wrote:
> > Does `getDefaultArgRange()` not provide the correct range already (so you don't need to go back to the original source)? Or does that range miss the `=`?
> > 
> > You might want to disable the fix-it in the case `getDefaultArgRange()` returns an empty range, or in case the default arg expression comes from a macro (and add a test for the latter case). e.g.,
> > ```
> > #define DERP(val)  = val
> > 
> > void f(int i DERP);
> > ```
> > Additionally, a test where the identifier is elided would be good as well: `void f(int = 12);`
> `getDefaultArgRange()` misses the `=` -- though if there's a better way to do it I'm all ears! 
In that case, this seems fine to me.


================
Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:31
+    diag(S->getParam()->getLocStart(),
+        "the default parameter was declared here",
+         DiagnosticIDs::Note);
----------------
Drop "the" from the diagnostic to match wording in other places


================
Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:44
+      diag(DefaultArgRange.getBegin(),
+            "the default parameter was declared here",
+            DiagnosticIDs::Note);
----------------
Drop "the" here as well.


================
Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:48-52
+      if (!D->getName().empty()) {
+       StartLocation = D->getLocation();
+      } else {
+       StartLocation = D->getLocStart();
+      }
----------------
This can be hoisted to the initialization: `SourceLocation StartLocation = D->getName().empty() ? D->getLocStart() : D->getLocation();`


================
Comment at: test/clang-tidy/fuchsia-default-arguments.cpp:40
+  return value;
+  // CHECK-MESSAGES: [[@LINE-2]]:12: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: the default parameter was declared here:
----------------
I think this diagnostic should be suppressed -- having it on the declaration where the default argument is defined should be sufficient, no?


================
Comment at: test/clang-tidy/fuchsia-default-arguments.cpp:57
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES-NOT: void h(int i);
----------------
Two more test cases that should be added:
```
void f(int i);
void f(int i = 12);

void f(int i) {
    
}

struct S {
  void f(int i);
};

void S::f(int i = 12) {}

int main() {
  S s;
  s.f();
  f();
}
```


https://reviews.llvm.org/D40108