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