NoQ accepted this revision.
NoQ added a comment.
LGTM, thanks!!
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1095-1102
// Gadgets "claim" variables they're responsible for. Once this loop
finishes,
// the tracker will only track DREs that weren't claimed by any gadgets,
// i.e. not understood by the analysis.
for (const auto &G : CB.FixableGadgets) {
for (const auto *DRE : G->getClaimedVarUseSites()) {
CB.Tracker.claimUse(DRE);
}
----------------
ziqingluo-90 wrote:
> NoQ wrote:
> > Let's also skip this part when there are no warning gadgets? Your initial
> > patch was clearing the `FixableGadgets` list so it was naturally skipped,
> > but now it's active again.
> Oh, I intentionally chose not to skip it:
> - 1. To keep the `Tracker` consistent with `CB.FixableGadgets` in case in
> the future we want to use these two objects even if there is no
> `WarningGadget`;
> - 2. The `findGadgets` function can stay as straightforward as its name
> suggests. It doesn't have to know the specific optimization for empty
> `WarningGadget`s.
>
> But the two thoughts above probably aren't important enough. I will change
> it back to skipping the loop when there is no warnings.
>
>
Hmm, I agree that this turns into a confusing contract. Maybe move this loop
out of the function, to the call site, so that it was more obvious that we skip
it? This would leave the tracker in a mildly inconsistent state at return, so
the caller will have to make it consistent by doing the claiming, but that's
arguably less confusing because we clearly communicate that this is an optional
step. So the function will do exactly what it says it'll do: find gadgets and
that's it.
Separately, we can probably consider not searching for fixable gadgets at all
when no warning gadgets are found. This could be a performance improvement, but
definitely a story for another time.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155524/new/
https://reviews.llvm.org/D155524
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
