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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
