[PATCH] D156762: [-Wunsafe-buffer-usage][NFC] Refactor `getFixIts`---where fix-its are generated

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I really like this!



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2178
+  // `FixItsForVariable` will map each variable to a set of fix-its directly
+  // associated to the variable itself.  Fix-its of distict variables in
+  // `FixItsForVariable` are disjoint.
----------------



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2243
+  // 1. overlap with macros or/and templates; or
+  // 2. conflicting each other.
+  // Otherwise, the fix-its will be dropped.
----------------



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2293
   // Instead, it should be visited along with the outer method.
+  // FIXME: do we want to do the same thing for `BlockDecl`s?
   if (const auto *fd = dyn_cast<CXXMethodDecl>(D)) {
----------------
Hmm, do we suffer from a crash similar to D150386 in presence of blocks instead of lambdas?

I vaguely remember that there were subtle differences, but I also think our approach should probably be the same, so this fixme is probably correct.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2526-2533
+  if (isa<FunctionDecl>(D))
+    // The only case where `D` is not a `FunctionDecl` is when `D` is a
+    // `BlockDecl`.  Let's NOT try to fix variables in blocks for now. Becuase
+    // those variables could be declared implicitly (captured variables) or in
+    // enclosing scopes.
+    FixItsForVariableGroup =
+        getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(),
----------------
A bit more idiomatic this way.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2527-2530
+    // The only case where `D` is not a `FunctionDecl` is when `D` is a
+    // `BlockDecl`.  Let's NOT try to fix variables in blocks for now. Becuase
+    // those variables could be declared implicitly (captured variables) or in
+    // enclosing scopes.
----------------
Ultimately we should accept `ObjCMethodDecl` here, and we should also accept `BlockDecl`s that live in global scope (where they can't be checked together with the surrounding function because there's no surrounding function).

Of course parameter fixits are going to be quite different in these cases, but it shouldn't stop us from trying. Even today, even though we don't know how to fix ObjC method parameters, we can already encounter cases where fixing local variables is sufficient.

In other words, I think `const FunctionDecl *` is the right parameter type for eg. `createFunctionOverloadsForParms()` ([[ https://reviews.llvm.org/D153059?id=539720#inline-1506914 | like I suggested before ]]), but probably not for the entire `getFixits()`,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156762