aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM once the optional bits are fixed up.
================
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;
----------------
whisperity wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > 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`?
> > 1. Maybe. Unfortunately, one needs to be careful with that. Originally I
> > did implement a "5th" heuristic that dealt with ignoring parameters that
> > had the same builtin operator used on them. However, while it silenced a
> > few false positives, it started creating **massive** amounts of false
> > negatives.
> > (In the concrete example, I think `foo = bar` will silence them because
> > //"appears in the same expression"//.)
> >
> > 2. I don't know. It would require a deeper understanding of Coroutines
> > themselves, and how people use them.
> If you don't mind me posting images, I can show a direct example. Things
> where the inability to track data flow really bites us in the backside.
>
> Examples from Apache Xerces:
>
> Here's a false positive that would be silenced by the logic of //"using the
> same operateur on the two params"//.
> {F15891567}
>
> And here is a false negative from the exact same logic.
> {F15891568}
>
> Maybe it could be //some// solace that we're restricting to
> non-const-lvalue-references (and pointers-to-non-const ??) and only
> assignment (**only** assignment?!))...
Ah, point #1 is an excellent point. As for #2, I think we can handle it in a
follow-up, but I believe `co_return` follows the same usage patterns as
`return`, generally.
================
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;
----------------
whisperity wrote:
> aaron.ballman wrote:
> > I don't think there's a need to make these optional when we're using the
> > `Enabled` flag.
> The original idea was that each heuristic could be individually toggled by
> the user... But I think we can agree that would be a bad idea and just
> overengineering.
Yeah, I think it's probably better to drop the optionals here -- we can revisit
the decision later if there's user feedback requesting finer granularity here.
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
