NoQ added inline comments.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:90
+ /// returns an empty list if no fixes are necessary.
+ virtual Optional<FixItList> getFixits(const Strategy &) const {
+ return None;
----------------
steakhal wrote:
> steakhal wrote:
> > I wonder if we should prefer `std::optional` as we are c++17.
> > IDK if it was ever covered on the community forum.
> > I'm just leaving this here without expecting any action.
> Per
> https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716/10,
> it seems like we should prefer std::optional.
Oooo nice.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:236
+ using UseSetTy = SmallSet<const DeclRefExpr *, 16>;
+ using DefMapTy = DenseMap<const VarDecl *, const DeclStmt *>;
+
----------------
xazax.hun wrote:
> NoQ wrote:
> > NoQ wrote:
> > > This extra payload wasn't advertised but it's very useful because it's
> > > hard to jump from `VarDecl` to `DeclStmt` without it, and we need to do
> > > such jump because our analysis starts from unsafe *uses* of a variable,
> > > which only give us a `VarDecl`.
> > Another way to look at this, is to notice that currently `DeclStmt` of the
> > variable isn't a `Gadget` on its own, and ask whether it *should* be a
> > `Gadget`.
> >
> > I currently believe that yes, it should be a `Gadget`, because it can
> > potentially be a "multi-variable" `Gadget` if, for example, the
> > initializer-expression is itself another raw pointer `DeclRefExpr`. So
> > there needs to be a way to communicate that the fix for the variable
> > declaration may depend on `Strategy` for more than one variable, and that
> > the `Strategy` itself should be chosen carefully, considering that choices
> > for different variables may interact this way. And from the point of view
> > of the *other* variable, this definitely needs to be a `Gadget`, it's no
> > different from any other `Gadget`.
> >
> > So, this part needs some more work.
> What about `DeclStmt`s that declare multiple variables, some safe some
> unsafe. Do you plan to generate fixits in that case exploding the DeclStmt?
Yes absolutely.
They're annoying because if the variable is in the middle, you'll get 3
`DeclStmt`s instead of one, but yeah, we'll have to handle those.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:259-262
+ // FIXME: Can this be less linear? Maybe maintain a map from VDs to DREs?
+ return any_of(*Uses, [VD](const DeclRefExpr *DRE) {
+ return DRE->getDecl()->getCanonicalDecl() == VD->getCanonicalDecl();
+ });
----------------
steakhal wrote:
> Maybe `Uses` should refer to the canonical decls. If that would be the case
> we could just call `.contains(VD)` here.
That's up to the AST, not up to us. I'd rather be safe here, because such bugs
are very hard to notice.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:428-430
+ for (const auto &Item : Map) {
+ const VarDecl *VD = Item.first;
+ const std::vector<const Gadget *> &VDGadgets = Item.second;
----------------
steakhal wrote:
>
I can get used to that!
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138253/new/
https://reviews.llvm.org/D138253
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
