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