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

[Alp Toker]

On 16/11/2013 01:32, Richard Smith wrote:
> On Fri, Nov 15, 2013 at 4:07 PM, Alp Toker <alp at nuanti.com 
> <mailto: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> <mailto: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.
 3. A tentative parse will yield the correct answer every time.


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.

> 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.

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/2edac633/attachment.html>