r183597 - Debug info: An if condition now creates a lexical scope of its own.

[Eric Christopher]

On Tue, Jul 9, 2013 at 2:33 AM, John McCall <rjmccall at apple.com> wrote:
> On Jun 17, 2013, at 2:32 PM, Eric Christopher <echristo at gmail.com> wrote:
>> On Mon, Jun 17, 2013 at 2:29 PM, Robinson, Paul
>> <Paul_Robinson at playstation.sony.com> wrote:
>>>> From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-
>>>> bounces at cs.uiuc.edu] On Behalf Of Eric Christopher
>>>> Sent: Monday, June 10, 2013 4:05 PM
>>>> To: Adrian Prantl
>>>> Cc: Nadav Rotem; cfe-commits at cs.uiuc.edu
>>>> Subject: Re: r183597 - Debug info: An if condition now creates a lexical
>>>> scope of its own.
>>>>
>>>> On Mon, Jun 10, 2013 at 3:56 PM, Adrian Prantl <aprantl at apple.com>
>>>> wrote:
>>>>> (CC'ing John because he understands the intricacies of LexicalScope
>>>> better than I do)
>>>>>
>>>>> On the first glimpse LexicalScope appears to be a subclass of
>>>> RunCleanupsScope that additionally emits a (DebugInfo-)LexicalScope. But
>>>> looking at the destructors it appears that they have slightly different
>>>> semantics: ~LexicalScope runs ForceCleanup and ~RunCleanupsScope
>>>> apparently doesn't.
>>>>>
>>>>> I'm wary that switching to Lexicalscope in
>>>> CodeGenFunction::EmitIfStmt() might lead to tricky ARC or EH-related
>>>> problems because of that.
>>>>>
>>>>> Does anyone have an opinion on that?
>>>>
>>>> That bit was added here:
>>>>
>>>> commit 495cfa46300979642acde8d93a1f21c9291dac98
>>>> Author: Nadav Rotem <nrotem at apple.com>
>>>> Date:   Sat Mar 23 06:43:35 2013 +0000
>>>>
>>>>    Make clang to mark static stack allocations with lifetime markers
>>>> to enable a more aggressive stack coloring.
>>>>    Patch by John McCall with help by Shuxin Yang.
>>>>    rdar://13115369
>>>>
>>>>
>>>> and oddly not to RunCleanupsScope.
>>>>
>>>> Nadav?
>>>>
>>>> -eric
>>>
>>> Looks like ~LexicalScope() just wants RunCleanupsScope::ForceCleanup()
>>> to happen before rescopeLabels().  All the right stuff happens in the right
>>> order if you change the RunCleanupsScope instance to LexicalScope.
>>> (RunCleanupsScope::ForceCleanup() does pretty much exactly the same thing
>>> as ~RunCleanupsScope() so it works out.  It could be done in a more obvious
>>> way, but functionally they're equivalent.)
>>>
>>
>> Agreed. I was just curious why Nadav changed one, but not the other.
>
> You're asking, why does only LexicalScope manage the set of labels to
> rescope?  Because RunCleanupsScope is specifically just supposed to
> manage the cleanup stack; it's not meant to do every possible thing that
> you might want to do with a language-level lexical scope.
>

*nod* That's fair. In this case we have a cleanup scope and a debug
lexical scope (though not enshrined as such).

> Specifically, rescoping labels isn't necessary for an arbitrary bundle of
> cleanups (like a full expression) because you can't typically jump into
> such things, and permitting that would involve more than just rescoping
> some labels.
>

I think this makes sense, but an example would probably illuminate.

> You really don't need a full LexicalScope here; all of the child
> expressions and statements are already being appropriately scoped.
> You should just make your own RAII class.
>

Oddly enough I did. It was called "LexicalScope" for things that
needed a debug lexical scope along with the assorted cleanups. It
seems to have been appropriated though :)

-eric