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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Ooo nice!!

> Extend PointerAssignmentGadget and PointerInitGadget to generate fix-its for cases where the left-hand side has won't fix strategy and the right-hand side has std::span strategy.

Hmm, is this intertwined with other changes, or is this completely separate? Also my head appears to be blank; did we reach any conclusion about the nature of "single" RHS-es and usefulness of annotating them as such? Which could be an approach that would encourage people to propagate more bounds in the backwards direction by default.



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1507
+    if (S.lookup(RightVD) == Strategy::Kind::Span) {
+      const ASTContext &Ctx = // FIXME: we need ASTContext to be passed in!
+          RightVD->getASTContext();
----------------
`MatchFinder::MatchResult.Context` is what you're probably looking for, so it's already kinda passed in. You can stash it in a member variable in the base class `Gadget` and have it readily available everywhere, i.e.

```lang=diff
 class Gadget {
 public:
-  Gadget(Kind K) : K(K) {}
+  Gadget(Kind K, const MatchResult &Result) : K(K), Context(*Result.Context) {
+    assert(Context);
+  }

 private:
   Kind K;
+  const ASTContext &Context;
 };

 class FixableGadget : public Gadget {
 public:
-  FixableGadget(Kind K) : Gadget(K) {}
+  FixableGadget(Kind K, const MatchResult &Result) : Gadget(K, Result) {}
 };

 class PointerAssignmentGadget {
   PointerAssignmentGadget(const MatchResult &Result)
-      : FixableGadget(Kind::PointerAssignment),
+      : FixableGadget(Kind::PointerAssignment, Result),
     PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
     PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}
 };
```
(the change in individual gadget classes is really tiny!)


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2688-2714
   // Remove a `FixableGadget` if the associated variable is not in the graph
   // computed above.  We do not want to generate fix-its for such variables,
   // since they are neither warned nor reachable from a warned one.
   //
   // Note a variable is not warned if it is not directly used in any unsafe
   // operation. A variable `v` is NOT reachable from an unsafe variable, if it
   // does not exist another variable `u` such that `u` is warned and fixing `u`
----------------
This entire loop can probably go away as soon as we stop relying on `FixablesForAllVars` and treat groups separately.

Then we can probably even have different Strategy objects for different groups, so `getNaiveStrategy()` could also be invoked per-group?


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2726
     FixItsForVariableGroup =
         getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(), D,
                   Tracker, Handler, VarGrpMgr);
----------------
I guess the next step is to call this function separately for each group?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157441