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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
