r194869 - PR8455: Handle an attribute between a goto label and a variable declaration per

[Richard Smith]

On Fri, Nov 15, 2013 at 8:31 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 16/11/2013 01:32, Richard Smith wrote:
>
> On Fri, Nov 15, 2013 at 4:07 PM, Alp Toker <alp at nuanti.com> wrote:
>
>>
>> On 16/11/2013 00:00, Alp Toker wrote:
>>
>>>
>>> On 15/11/2013 23:42, Richard Smith wrote:
>>>
>>>> On Fri, Nov 15, 2013 at 3:33 PM, Alp Toker <alp at nuanti.com <mailto:
>>>> alp at nuanti.com>> wrote:
>>>>
>>>>     Hello Richard,
>>>>
>>>>     For the record I had a patch up for review to fix this back from
>>>>     2010, not sure if it ever reached the list:
>>>>
>>>>     http://llvm.org/bugs/show_bug.cgi?id=8455
>>>>
>>>>     Just to compare notes, my approach was to use a
>>>>     TentativeParsingAction to handle some of the corner cases.
>>>>
>>>>
>>>> Aaron's original patch went in that direction, but got reworked to
>>>> avoid the cost of a tentative parse.
>>>>
>>>
>>> But this *is* a disambig problem. The tentative parse can yield a more
>>> meaningful diag when encountering Microsoft-style attributes, such as those
>>> appearing in some MSVC headers. This outweighs the theoretical cost of a
>>> tentative parse IMO.
>>>
>>
>>  In fact, this applies not just to Microsoft-style attributes but also
>> Objective-C messages using square brackets.
>>
>> Tentative parse was the only solution to handle this correctly in
>> Objective-C++ using GNU labeled statements with C++0x attributes when the
>> patch was written three years ago, and although the standards weren't final
>> yet I don't think that's changed.
>>
>> Have you actually tested corner cases like this?
>
>
>  The patch only applies to GNU attributes; they're the only ones we parse
> at this location. If there's a problem with __declspec attributes here,
> it's a different problem, since we don't look for them here.
>
>
> Come on Richard, you know there's a problem here because there's a big
> FIXME added it in the patch you landed:
>
>
> +    else if (isDeclarationStatement()) {
> +      StmtVector Stmts;
> *+      // FIXME: We should do this whether or not we have a declaration*
> +      // statement, but that doesn't work correctly (because
> ProhibitAttributes
>
> It's obvious from inspection that:
>
>
>    1. Non-declaration statements can end up here.
>    2. isDeclarationStatement() isn't enough to detect everything a GNU
>    label can be applied to in this context.
>
>
Got a counterexample?


>    1. A tentative parse will yield the correct answer every time.
>
> The bug is that ProhibitAttributes doesn't work, so we do the
'isDeclarationStatement' dance unnecessarily.


> What would we lose from doing a tentative parse here? The mechanism to
> handle lexical ambiguity is there for a reason, used extensively elsewhere
> and works for all supported language standards / extensions without the
> need for confusing special cases and FIXMEs.
>

We avoid tentative parses where possible, for performance reasons.

  There's no problem with C++11 attributes or square brackets, since they
> don't go in this location when applied to a label. Your patch also only
> applied to GNU attributes.
>
>    Judging that PR8455 was closed shortly after the commit, I'm guessing
>>> you or Aaron knew my patch existed beforehand.
>>>
>>> Please run it by me next time if a fix is already out there and you
>>> think there's a better approach so we can discuss it.
>>>
>>
>  I only saw your patch after the fix was already committed. Our usual
> workflow does not involve posting patches to bugs; such patches will
> usually be ignored, since bug updates don't generate mail to llvmbugs.
> That's arguably a problem with the way bugzilla is set up...
>
>
> Our usual workflow is to take at least a cursory look at the PR we
> discussed in the commit message, and in the patch itself.
>
> (But Alp from three years ago says thanks for the tip!)
>
>
>   In any case, your approach had been discussed and we chose to go a
> different way, so even if we'd seen your patch it probably wouldn't have
> made much difference (except that I might have cc'd you on the review --
> but I don't think it's reasonable of you to demand that treatment, since
> you can read this list as well as anyone else can).
>
>
> Not demanding anything, just pointing out that your patch is wrong.
>

You've not actually pointed that out yet. I'm still waiting for a testcase.


> Cheers,
> Alp.
>
>
> -- http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131116/6d955657/attachment.html>