li.zhe.hua marked an inline comment as done.
li.zhe.hua 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) {
----------------
ymandel wrote:
> 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.
> I'm afraid this may allow us to hide a bug.
Ah, yes, that is definitely true.
> That said, any idea if we actually need to use `Reference` skipping at all?
Whatever we choose, I imagine it should be kept the same as the analogous line
in [[
https://github.com/llvm/llvm-project/blob/118c5d1c97b4191678663bf2a938eee7dec6f0b1/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp#L112-L113
| extendFlowCondition ]]? I've opted to not change it for now, given my
uncertainty.
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
