[PATCH] D157441: [-Wunsafe-buffer-usage] Use `Strategy` to determine whether to fix a parameter

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added inline comments.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2586
 #endif
         it = FixablesForAllVars.byVar.erase(it);
       } else if (Tracker.hasUnclaimedUses(it->first)) {
----------------
We cannot fix reference type variable declarations for now.  
Besides, it seems that reference type is invisible to `Expr`s.  For example,
```
void foo(int * &p) {
      p[5];  // the type of the DRE `p` is a pointer-to-int instead of a reference
}
```
So we ignore such variables explicitly here.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2719
+  Strategy NaiveStrategy = getNaiveStrategy(
+      llvm::make_range(VisitedVars.begin(), VisitedVars.end()));
   VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap, GrpsUnionForParms);
----------------
t-rasmud wrote:
> ziqingluo-90 wrote:
> > `VisitedVars` are the set of variables on the computed graph.  The previous `UnsafeVars` is a superset of it, including safe variables that have `FixableGadget`s discovered.  We do not want to assign strategies other than `Won't fix` to those safe variables.
> Would it make more sense (make the code more readable) if we renamed `UnsafeVars` to say, `FixableVars`?
Oh, I just realized that `UnsafeVars` is now an unused variable.  So let's remove it. `FixableVars` would be a better name for sure, since it is in fact the key set of `FixablesForAllVars.byVar`.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2719
+  Strategy NaiveStrategy = getNaiveStrategy(
+      llvm::make_range(VisitedVars.begin(), VisitedVars.end()));
   VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap, GrpsUnionForParms);
----------------
ziqingluo-90 wrote:
> t-rasmud wrote:
> > ziqingluo-90 wrote:
> > > `VisitedVars` are the set of variables on the computed graph.  The previous `UnsafeVars` is a superset of it, including safe variables that have `FixableGadget`s discovered.  We do not want to assign strategies other than `Won't fix` to those safe variables.
> > Would it make more sense (make the code more readable) if we renamed `UnsafeVars` to say, `FixableVars`?
> Oh, I just realized that `UnsafeVars` is now an unused variable.  So let's remove it. `FixableVars` would be a better name for sure, since it is in fact the key set of `FixablesForAllVars.byVar`.
To correct myself, `VisitedVars` are the variables In the graph but it may not be a subset of "fixable variables".  `VisitedVars` may contain variables that is not fixable, i.e., variables having no `FixableGadget` associated with.  We should leave those unfixable variables being WON'T FIX.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157441/new/

https://reviews.llvm.org/D157441