ymandel added inline comments.
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:574
- // Sub-expressions that are logic operators are not added in basic blocks
- // (e.g. see CFG for `bool d = a && (b || c);`). If `SubExpr` is a logic
- // operator, it isn't evaluated and assigned a value yet. In that case, we
- // need to first visit `SubExpr` and then try to get the value that gets
- // assigned to it.
- Visit(&SubExpr);
- if (auto *Val = dyn_cast_or_null<BoolValue>(
- Env.getValue(SubExpr, SkipPast::Reference)))
+ auto *SubExprLoc = Env.getStorageLocation(SubExpr, SkipPast::Reference);
+ if (SubExprLoc == nullptr) {
----------------
I'm afraid this may allow us to hide a bug. Specifically: consider if
`SubExpr` was already evaluated to a RefenceValue and that value's
StorageLocation does *not* map to a value. Then, this line will be true, but we
won't want to revisit the `SubExrp`. Now, that may be impossible in practice,
so I'm not sure how big a problem this is, but it seems safer to just use
`SkipPast::None` and not rely on any guarantees.
That said, i see why you want the uniformity of `Reference` because it is used
below. That said, any idea if we actually need to use `Reference` skipping at
all? I think so -- the case of a variable or field access as sub-expression,
but those probably have a cast around them anyhow, so i'm not sure those will
require reference skipping.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125821/new/
https://reviews.llvm.org/D125821
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
