[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/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:30
+  /// of primitive fixits (individual insertions/removals/replacements).
+  using FixItList = llvm::SmallVector<FixItHint, 2>;
+
----------------
steakhal wrote:
> Unless we have a well-formed idea of how many elements we are going to have, we should probably omit it.
Better yet, use a `SmallVectorImpl` (since this shows up in an interface)


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11654-11657
+def warn_unsafe_buffer_expression : Warning<"unchecked operation on raw buffer in expression">,
+  InGroup<UnsafeBufferUsage>, DefaultIgnore;
+def warn_unsafe_buffer_variable : Warning<"variable %0 participates in unchecked buffer operations">,
+  InGroup<UnsafeBufferUsage>, DefaultIgnore;
----------------



================
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>;
----------------
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.


================
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;
+      }
+    }
----------------
Use a `specific_decl_iterator<VarDecl>` instead?


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:329-335
+      if (const auto *DRE = Result.Nodes.getNodeAs<DeclRefExpr>("any_dre")) {
+        Tracker.discoverUse(DRE);
+      }
+
+      if (const auto *DS = Result.Nodes.getNodeAs<DeclStmt>("any_ds")) {
+        Tracker.discoverDecl(DS);
+      }
----------------



================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2161-2163
+    for (const auto &F: Fixes) {
+      D << F;
+    }
----------------
Hmmm... `llvm::for_each` instead?


Repository:
  rC Clang

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

https://reviews.llvm.org/D138253