[PATCH] D138253: [-Wunsafe-buffer-usage] NFC: Implement fix-strategies and variable-use-claiming.

[Artem Dergachev via Phabricator via cfe-commits]

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