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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
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:
> 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.


================
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:
> 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.


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

https://reviews.llvm.org/D78652