[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Alright, I think I managed to fully chew through this patch and I think it's beautiful and everything makes sense to me!

I have a tiny complaint though: It is very large though, 500 lines of code is very hard to digest all at once. Because we aren't coming in with all the context you had when you wrote this code, it's often very hard for us readers to guess the intention behind every section of the diff, and how they are supposed to work together.

In this patch there are a few refactoring steps that weren't responsible for any change in behavior:

- Introduce the `VariableGroupsManager` abstraction;
- Extract `eraseVarsForUnfixableGroupMates()` into a function;
- Extract `listVariableGroupAsString` into a function;

But because I didn't know that at first, I had to carefully look at every line of code to confirm that it actually doesn't change anything.

So it'd help tremendously if these refactorings were extracted into a parallel "no functional change" ("NFC") patch (with zero tests: no failures on existing tests => the patch is indeed NFC) that doesn't do anything other than "prepare the codebase" for this patch. Then in the NFC patch we'd discuss architecture and confirm that it truly doesn't change anything, whereas in this patch every line of code would be filled with meaning and purpose! ^.^

I'm not sure it makes sense split it up now, probably depends on whether Jan and Rashmi have the same troubles as me ^.^

I also has nitpicks.



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2130-2131
+                     [&FixItsForVariable](const VarDecl *GrpMember) -> bool {
+                       return FixItsForVariable.find(GrpMember) ==
+                              FixItsForVariable.end();
+                     })) {
----------------
(`.contains()` is unfortunately C++20)


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2156-2157
+
+  if (auto *FD = dyn_cast<FunctionDecl>(
+          D)) { // If `D` is not a `FunctionDecl`, no parameter will be fixed.
+    std::vector<bool> ParmsNeedFixMask(FD->getNumParams(), false);
----------------
Would it make sense to make `createFunctionOverloadsForParms` accept a `FunctionDecl *D` from the start?


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2162-2163
+    for (unsigned i = 0; i < FD->getNumParams(); i++)
+      if (FixItsForVariable.find(FD->getParamDecl(i)) !=
+          FixItsForVariable.end()) {
+        ParmsNeedFixMask[i] = true;
----------------
Another good use for `.count()`.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2224
+  // `FixItsForVariable` now contains only variables that can be
+  // fixed. A variable can be fixed if its' declaration and all Fixables
+  // associated to it can all be fixed.
----------------



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2430
+  // The union group over the ones in "Groups" that contain parameters of `D`:
+  llvm::SetVector<const VarDecl *>
+      GrpsUnionForParms; // these variables need to be fixed in one step
----------------
Why `SetVector`? Do we care about order? Maybe just `SmallSet`?


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2171
+    if (VarGroupForVD.size() <= 1)
+      return "";
+
----------------
Does the empty string actually ever make sense for the caller? Maybe just assert that this never happens, and force the caller to check that?


================
Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp:37-39
+// CHECK: fix-it:{{.*}}:{[[@LINE+3]]:24-[[@LINE+3]]:30}:"std::span<int> p"
+// CHECK: fix-it:{{.*}}:{[[@LINE+2]]:32-[[@LINE+2]]:39}:"std::span<int *> q"
+// CHECK: fix-it:{{.*}}:{[[@LINE+1]]:41-[[@LINE+1]]:48}:"std::span<int> a"
----------------
The fix is emitted 3 times right? Once for each warning?

Would it make sense to duplicate the three `CHECK:` lines three times to confirm that?


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

https://reviews.llvm.org/D153059