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

[Balázs Benics via Phabricator via cfe-commits]

steakhal 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>;
+
----------------
Unless we have a well-formed idea of how many elements we are going to have, we should probably omit it.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:83
+  /// Returns the list of pointer-type variables on which this gadget performs
+  /// its operation. Typically there's only one variable. This isn't a list
+  /// of all DeclRefExprs in the gadget's AST!
----------------



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


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:231-233
+// An auxiliary tracking facility for the fixit analysis. It helps connect
+// declarations to its and make sure we've covered all uses with our analysis
+// before we try to fix the declaration.
----------------



================
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();
+    });
----------------
Maybe `Uses` should refer to the canonical decls. If that would be the case we could just call `.contains(VD)` here.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:408
   for (const auto &G : Gadgets) {
-    Handler.handleUnsafeOperation(G->getBaseStmt());
+    DeclUseList DREs = G->getClaimedVarUseSites();
+
----------------



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



Repository:
  rC Clang

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

https://reviews.llvm.org/D138253