[clang] [alpha.webkit.UncountedLocalVarsChecker] Allow uncounted object references within trivial statements (PR #82229)

Artem Dergachev via cfe-commits cfe-commits at lists.llvm.org
Thu Feb 29 15:18:16 PST 2024


================
@@ -171,6 +151,24 @@ class UncountedLocalVarsChecker
 
     std::optional<bool> IsUncountedPtr = isUncountedPtr(ArgType);
     if (IsUncountedPtr && *IsUncountedPtr) {
+
+      ASTContext &ctx = V->getASTContext();
+      for (DynTypedNodeList ancestors = ctx.getParents(*V); !ancestors.empty();
+           ancestors = ctx.getParents(*ancestors.begin())) {
----------------
haoNoQ wrote:

I don't think this should be a recursive loop. As is, it'll match `x` not only in
```
if (int x = 1) {
}
```
but also in
```
if (1) {
  int x;
}
```
which is probably not what you want(?) So you probably need to check that `V` is the condition variable.

Also, generally speaking, usually we try to avoid `ParentMap`/`getParent()` shenanigans. Parent lookups aren't naturally provided by the AST, they rely on building the "parent map" by scanning the entire AST in the natural order and putting all edges into the map. There are a few very confusing cornercases in this algorithm and a few known crashes in that I honestly don't know how to fix. It also screws algorithmic complexity because now we have an additional backwards traversal; it's very easy to accidentally start walking back and forth indefinitely, or just way too many times. Sometimes it's necessary but most of the time there's a normal top-down approach that gives the answer directly.

In this case I think it's better to redefine eg. `VisitIfStmt()`/`TraverseIfStmt()`to manually traverse the condition variable in a custom manner, and then suppress the automatic traversal (or just traverse the initializer). That's clearly O(1) and doesn't rely on any unreliable technology.

https://github.com/llvm/llvm-project/pull/82229


More information about the cfe-commits mailing list