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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Ok I think this looks great and almost good to go. I love how the core change in the grouping algorithm is actually tiny and simple!



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1891-1892
+  for (unsigned i = 0; i < NumParms; i++) {
+    if (!ParmsMask[i])
+      continue;
+
----------------
ziqingluo-90 wrote:
> NoQ wrote:
> > Speaking of [[ https://reviews.llvm.org/D156762#inline-1517322 |my comment on the other patch]].
> > 
> > Can we simply ask `Strategy` about the strategy for `FD->getParamDecl(i)` instead of passing a mask?
> > 
> > Eventually we'll have to do that anyway, given how different parameters can be assigned different strategies.
> Yes.  I think using `Strategy` is the correct way to do it.  
> 
> Can I make it a follow-up patch?  I noticed that this line needs to be fixed:
> ```
>   Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);
> ```
> The `UnsafeVars` is the set of variables associated to all matched `FixableGadget`s.  There can be variables that is safe and do not need fix.  Their strategy is set to `std::span` currently which is not correct.  With this line being fixed, we can have examples like
> ```
> void f(int * p) {
>   int * q;
>    q = p;
>    p[5] = 5;
> }
> ```
> where `q`'s strategy is `WONTFIX` and `p`'s strategy is `std::span`.  The expression `q = p` can be fixed to `q = p.data()`.  It  currently has an empty fix-it, which is incorrect.
Yes sure!


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2364-2366
+    if (!VarGrpMap.count(Var))
+      return std::nullopt;
+    return Groups[VarGrpMap.at(Var)];
----------------
The usual idiom to avoid double lookup: use `find()` to extract an iterator, compare it to `end()` to see if the item was found, and if it was then dereference the iterator.

I wouldn't use `at()` given that the whole point of `.at()` is to throw exceptions, while LLVM is compiled with `-fno-exceptions`.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2281
       FD << listVariableGroupAsString(Variable, VarGroupForVD)
-         << (VarGroupForVD.size() > 1);
+         << (VarGroupForVD.size() > 1) << ND->getNameAsString();
       for (const auto &F : Fixes) {
----------------
IIRC we're supposed to stuff `ND` directly into the stream and it'll figure out on its own how to name it, or how to put quotes around it. See how there's no quotes around `%2`!


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

https://reviews.llvm.org/D153059