================
@@ -474,4 +504,22 @@ bool TrivialFunctionAnalysis::isTrivialImpl(
return Result;
}
+bool TrivialFunctionAnalysis::isTrivialImpl(
+ const Stmt *S, TrivialFunctionAnalysis::CacheTy &Cache) {
+ // If the statement isn't in the cache, conservatively assume that
+ // it's not trivial until analysis completes. Unlike a function case,
+ // we don't insert an entry into the cache until Visit returns
+ // since Visit* functions themselves make use of the cache.
----------------
haoNoQ wrote:
> ```
> // If the statement isn't in the cache, conservatively assume that
> // it's not trivial until analysis completes.
> ```
This comment should probably be moved to `WithCachedResult()` where the
false-by-default insertion actually takes place. (Not sure if it was clear that
my original comment was talking about that insertion specifically. If it wasn't
clear, it was probably a bad comment that needed to be phrased differently in
the other function as well.)
> ```
> // Unlike a function case,
> // we don't insert an entry into the cache until Visit returns
> // since Visit* functions themselves make use of the cache.
> ```
We can probably avoid double insert/lookup by skipping this insert/lookup
entirely. Just rely on `Visit()` to do the bookkeeping.
But at the same time, it's nice that your code guarantees *some* caching even
when we forget to cache a specific statement. It's a property we'd lose if we
simply remove this global insert-lookup.
So I wonder if you can replace it with an *assertion* instead? Something like:
```
bool Result = V.Visit(S);
assert(Cache.contains(S) && "Top-level statement not properly cached!");
return Result;
```
This may actively help us find forgotten `WithCachedResult()` calls instead of
silently accepting them.
https://github.com/llvm/llvm-project/pull/82229
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
