[PATCH] D138329: [-Wunsafe-buffer-usage] Add a new recursive matcher to replace `forEachDescendant` in unsafe buffer check

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D138329#3938351 <https://reviews.llvm.org/D138329#3938351>, @xazax.hun wrote:

> Is the problem `forEachDescendant` matching statements inside blocks and lambdas? I wonder if this behavior would surprise people, so I think it would be better to:
>
> - Potentially add a template bool parameter to `forEachDescendant` controlling this behavior.

Yeah, I honestly think that it is the current behavior of `forEachDescendant` that's counterintuitive and almost never does what the user actually wants. People typically get surprised when it *does* descend into callables, something they didn't even think about.

And the idiom

  decl(forEachDescendant(..., forCallable(equalsBoundNode("D"))).bind("D")

is not only clumsy and obscure but also algorithmically slower.

We do need a better name though, the difference between "Each" and "Every" isn't exactly the same as the difference between this matcher's behavior and that matcher's behavior. I was thinking of `forEachStmtDescendant`, as in it matches "statement-descendants". An even better name could be `forEachDescendantInCallable` which describes the behavior precisely and also connects to the old idiom for discoverability.

> - Review existing uses because I am not entirely sure if the current behavior is the right default.

Yeah that's definitely a good idea.



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:50-51
+
+  // For the matchers so far used in spanification, we only need to match
+  // `Stmt`s.  To override more as needed.
+
----------------
^^


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:59
+    // To skip callables:
+    if (llvm::isa<FunctionDecl, BlockDecl, ObjCMethodDecl>(Node))
+      return true;
----------------
`llvm::` is unnecessary, we're under `using namespace llvm`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138329