[PATCH] D78652: [clang-tidy] Suppress reports to similarly used parameters in 'bugprone-easily-swappable-parameters'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:529
+/// ReturnStmts return them from the function.
+class Returned {
+  llvm::SmallVector<const ParmVarDecl *, SmallDataStructureSize> ReturnedParams;
----------------
Question: would it make sense to (someday, not now) consider output parameters similar to return statements? e.g.,
```
void int_swap(int &foo, int &bar) {
  int temp = foo;
  foo = bar;
  bar = temp;
}
```
As a question for today: should `co_return` should be handled similarly as `return`?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:563-566
+  llvm::Optional<relatedness_heuristic::AppearsInSameExpr> SameExpr;
+  llvm::Optional<relatedness_heuristic::PassedToSameFunction> PassToFun;
+  llvm::Optional<relatedness_heuristic::AccessedSameMemberOf> SameMember;
+  llvm::Optional<relatedness_heuristic::Returned> Returns;
----------------
I don't think there's a need to make these optional when we're using the `Enabled` flag.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:481
+
+      assert(TargetIdx.hasValue() && "Matched, but didn't find index?");
+      TargetParams[PassedParamOfThisFn].insert(
----------------
whisperity wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > I *think* you could run into this assert with K&R C function where there is a `FunctionDecl` to get back to but the decl claims it has no params because it has no prototype (while the call expression actually has the arguments). However, there may be other code that protects us from this case.
> > At face value, I would say the fact that a K&R function //has no params// declared means that the matcher above will not be able to do `forEachArgumentWithParam`, because the argument vector is N long, but the parameter vector is empty.
> > 
> > Nevertheless, it won't hurt to add a test case for this though.
> No assertion, no crash. But unfortunately, we generate a false positive. But we don't crash. I added a test for this, just in case.
Thanks for adding the test case! I think a false positive is reasonable enough given how rarely I expect people using K&R C function declarations will worry about swapped arguments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:532-536
+      if (find(FD->parameters(), ReturnedParam) == FD->param_end())
+        // Returning an Expr to a ParmVarDecl that **isn't** parameter of the
+        // function should not be possible. Make sure a mistake of the matcher
+        // does **not** interfere with the heuristic's result.
+        continue;
----------------
whisperity wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > Is this a FIXME because there's a matcher issue this works around or should it be an assertion?
> > I was unable to create a test for this case. This part of the code, while I was twisting my head left and right, is one of the strictest remnant of the old version of the check... There definitely was a project in the test set back in December 2019 which made the check crash or emit massive false positives, and this thing was added to guard against that.
> > 
> > At a hunch, I think //maybe// a lambda returning one of its captures (which are, AFAIK, `ParmVarDecl`s of the constructor!) could be what caused the crash/false positive. I will check this out in detail soon.
> I do not remember why I thought this makes crashes because I tried and no crashing now (which is good), but lambdas defined inside a function's body is definitely a good reproducer for when this is hit.
> 
> Rephrased the comment, and added a test about this.
Thank you for the clarification and test coverage!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78652/new/

https://reviews.llvm.org/D78652