xazax.hun added a subscriber: dcoughlin.
xazax.hun added inline comments.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:616
+ bool erased = DeclToLoc.erase(&D);
+ if (!erased)
+ D.dump();
----------------
Would we dump this in release builds? Do we want to limit this to debug/assert
builds?
================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:329
+static void
+builtinTransferScopeEnd(const CFGScopeEnd &Elt,
+ TypeErasedDataflowAnalysisState &InputState) {
----------------
I think one question is whether we are interested in ScopeEnd or LifetimeEnd,
see the discussions in https://reviews.llvm.org/D15031 and
https://reviews.llvm.org/D16403, specifically Devin's comment:
>>! In D16403#799926, @dcoughlin wrote:
>> @dcoughlin As a reviewer of both patches - could you tell us what's the
>> difference between them? And how are we going to resolve this issue?
>
> These two patches are emitting markers in the CFG for different things.
>
> Here is how I would characterize the difference between the two patches.
>
> - Despite its name, https://reviews.llvm.org/D15031, is really emitting
> markers for when the lifetime of a C++ object in an automatic variable ends.
> For C++ objects with non-trivial destructors, this point is when the
> destructor is called. At this point the storage for the variable still
> exists, but what you can do with that storage is very restricted by the
> language because its contents have been destroyed. Note that even with the
> contents of the object have been destroyed, it is still meaningful to, say,
> take the address of the variable and construct a new object into it with a
> placement new. In other words, the same variable can have different objects,
> with different lifetimes, in it at different program points.
>
> - In contrast, the purpose of this patch (https://reviews.llvm.org/D16403) is
> to add markers for when the storage duration for the variable begins and ends
> (this is, when the storage exists). Once the storage duration has ended, you
> can't placement new into the variables address, because another variable may
> already be at that address.
>
> I don't think there is an "issue" to resolve here. We should make sure the
> two patches play nicely with each other, though. In particular, we should
> make sure that the markers for when lifetime ends and when storage duration
> ends are ordered correctly.
What I wanted to add, I wonder if we might not get ScopeEnd event for
temporaries and there might be other differences. The Clang implementation of
P1179 used LifetimeEnd instead of ScopeEnd, and I believe probably most clients
are more interested in LifetimeEnd events rather than ScopeEnd.
I think I understand why you went with ScopeEnd for this specific problem, but
to avoid the confusion from having both in the Cfg (because I anticipate
someone will need LifetimeEnd at some point), I wonder if we can make this work
with LifetimeEnd.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149144/new/
https://reviews.llvm.org/D149144
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
