ziqingluo-90 added inline comments.
================
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);
}
----------------
NoQ wrote:
> 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.
Thanks for the suggestion. I moved the loop to the end of the call to
`findGadgets` and landed it.
Repository:
rG LLVM Github Monorepo
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
