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