[PATCH] D155524: [-Wunsafe-buffer-usage] Ignore the FixableGadgets that will not be fixed at an earlier stage

[Ziqing Luo via Phabricator via cfe-commits]

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