[clang] [clang][dataflow] Add `Environment::get<>()`. (PR #76027)

via cfe-commits cfe-commits at lists.llvm.org
Wed Dec 20 23:40:47 PST 2023


================
@@ -489,8 +489,7 @@ Environment Environment::pushCall(const CallExpr *Call) const {
   if (const auto *MethodCall = dyn_cast<CXXMemberCallExpr>(Call)) {
     if (const Expr *Arg = MethodCall->getImplicitObjectArgument()) {
       if (!isa<CXXThisExpr>(Arg))
-          Env.ThisPointeeLoc =
-              cast<RecordStorageLocation>(getStorageLocation(*Arg));
+        Env.ThisPointeeLoc = get<RecordStorageLocation>(*Arg);
----------------
martinboehme wrote:

> should we check the result here, since the original code had a plain cast?

Thanks, good catch!

I've chosen to revert this for the moment, as I'm not sure `get<>(); asssert();` is better than the existing `cast<>(getStorageLocation())`. If anything, we probably want an additional convenience function for this (see below).

> +1, alternatively we could introduce a bool template argument that would determine whether `null` is allowed with a default value.

Yes, we probably want some mechanism to express the combined effect of `cast<>(getStorageLocation())` too (i.e. disallowing null).

I'm not sure a bool argument is the best choice though -- as it's often hard to tell at the callsite what it means:

```cxx
auto *Val = Env.get<PointerValue, true /* 🤔 */>(E);
```

As an alternative, we could use an enum with a more self-documenting name, but at that point we could also simply introduce a different routine. I haven't yet found a great name for this that I'm satisfied with -- `getMustExist()` is the best I can come up with, but it's long and clumsy. Any suggestions?

Anyway, I'll leave this for future work.

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


More information about the cfe-commits mailing list