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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:17-19
+// Because the analysis revolves around variables and their types, we'll need to
+// track uses of variables (aka DeclRefExprs).
+using DeclUseList = SmallVector<const DeclRefExpr *, 1>;
----------------
NoQ wrote:
> aaron.ballman wrote:
> > Do we have to care about data member accesses (which would be a `MemberExpr` and not a `DeclRefExpr`)?
> > 
> > Also, because this shows up in interfaces, it should probably be a `SmallVectorImpl` so the size doesn't matter.
> > Do we have to care about data member accesses (which would be a `MemberExpr` and not a `DeclRefExpr`)?
> 
> Unlikely. Auto-fixing member variables without breaking source or binary compatibility is next to impossible.
> 
> > Also, because this shows up in interfaces, it should probably be a SmallVectorImpl so the size doesn't matter.
> 
> This doesn't seem to work on return types, which is where these classes show up in my case (`SmallVectorImpl` has deleted copy-move constructors so it's only suitable for passing by reference).
>>Do we have to care about data member accesses (which would be a MemberExpr and not a DeclRefExpr)?
> Unlikely. Auto-fixing member variables without breaking source or binary compatibility is next to impossible.

Okie dokie!

>> Also, because this shows up in interfaces, it should probably be a SmallVectorImpl so the size doesn't matter.
> This doesn't seem to work on return types, which is where these classes show up in my case (SmallVectorImpl has deleted copy-move constructors so it's only suitable for passing by reference).

Ahhh I had forgotten that we didn't allow it as a move-only type.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:266-271
+    for (const Decl *D: DS->decls()) {
+      if (const auto *VD = dyn_cast<VarDecl>(D)) {
+        assert(Defs.count(VD) == 0 && "Definition already discovered!");
+        Defs[VD] = DS;
+      }
+    }
----------------
NoQ wrote:
> aaron.ballman wrote:
> > Use a `specific_decl_iterator<VarDecl>` instead?
> Hmm `DeclStmt` doesn't seem to provide a neat accessor, would you like me to add it?
Eh, I don't insist. Feel free to do it as an NFC change after landing this patch, if you want to.


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

https://reviews.llvm.org/D138253