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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
