ymandel accepted this revision. ymandel added a comment. SGTM
================ Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:545 + &cast<AggregateStorageLocation>(State.Env.createStorageLocation(*E)); + State.Env.setStorageLocationStrict(*E, *Loc); + } ---------------- mboehme wrote: > ymandel wrote: > > Should this line instead be outside of this `if`? So that the flow is > > ``` > > Loc = get > > if (!Loc) Loc = create > > set(Loc) > > ``` > I don't understand -- wouldn't this just be doing unnecessary work? > > If > > ``` > Loc = cast_or_null<AggregateStorageLocation>( > State.Env.getStorageLocationStrict(*E)); > ``` > > returns a non-null `Loc`, we don't create a new `Loc`, hence > > ``` > State.Env.setStorageLocationStrict(*E, *Loc); > ``` > > is a no-op, as `E` is already associated with `Loc`. Right, I was not paying enough attention. I was thinking specifically about the value. basically, i confused line 545 for line 549. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156672/new/ https://reviews.llvm.org/D156672 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
