================
@@ -636,40 +636,37 @@ class OptionalIntAnalysis final
if (!CS)
return;
const Stmt *S = CS->getStmt();
- auto OptionalIntRecordDecl = recordDecl(hasName("OptionalInt"));
- auto HasOptionalIntType = hasType(OptionalIntRecordDecl);
-
- SmallVector<BoundNodes, 1> Matches = match(
- stmt(anyOf(cxxConstructExpr(HasOptionalIntType).bind("construct"),
- cxxOperatorCallExpr(
- callee(cxxMethodDecl(ofClass(OptionalIntRecordDecl))))
- .bind("operator"))),
- *S, getASTContext());
- if (const auto *E = selectFirst<CXXConstructExpr>(
- "construct", Matches)) {
- cast<RecordValue>(Env.getValue(*E))
- ->setProperty("has_value", Env.getBoolLiteralValue(false));
- } else if (const auto *E =
- selectFirst<CXXOperatorCallExpr>("operator", Matches)) {
- assert(E->getNumArgs() > 0);
- auto *Object = E->getArg(0);
- assert(Object != nullptr);
-
- refreshRecordValue(*Object, Env)
- .setProperty("has_value", Env.getBoolLiteralValue(true));
+ const Expr *E = dyn_cast<Expr>(S);
+ if (!E)
+ return;
+
+ if (!E->getType()->isPointerType())
+ return;
+
+ // Make sure we have a `PointerValue` for `E`.
+ auto *PtrVal = cast_or_null<PointerValue>(Env.getValue(*E));
----------------
martinboehme wrote:
Agree that divergence is a concern -- but I do think we need to let checks create their own values when the framework doesn't. Here's an [example](https://github.com/google/crubit/blob/d63080a7335ae2babb2f6c48898dc8a680a7d017/nullability/pointer_nullability_analysis.cc#L1355) of where we do this in Crubit's nullability check (i.e. in production code) to ensure that every expression of pointer type is associated with a value.
This is needed because the framework itself doesn't create `Value`s for every expression. As just one example, the framework doesn't create values for `CallExpr`s, but we definitely need values for these in the nullability check as a function returning a pointer can have nullability annotations on that pointer, and we then need to propagate this pointer value, along with its nullability information.
We could change this by making sure that the framework creates `Value`s for every expression, but this would come at a cost: We would be creating a lot of values that any given analysis doesn't need, and this would negatively impact runtime, memory consumption, and potentially convergence.
AIUI from @gribozavr, the framework's philosophy so far has been to provide base functionality that is likely to be needed by most or all analyses, and it lets a specific analysis customize or extend the behavior where the existing behavior is not sufficient. But this does come with a tradeoff; as you point out, a badly written analysis can trigger divergence and other unwanted behavior. IOW, we're relying on the authors of the analysis to know what they're doing.
@gribozavr Do you have additional thoughts you want to add here?
(For the purposes of this patch, I don't think there's anything actionable here, so I will go ahead and submit.)
https://github.com/llvm/llvm-project/pull/76042