On Fri, Nov 15, 2013 at 8:31 PM, Alp Toker <[email protected]> wrote: > > On 16/11/2013 01:32, Richard Smith wrote: > > On Fri, Nov 15, 2013 at 4:07 PM, Alp Toker <[email protected]> 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 <[email protected] <mailto: >>>> [email protected]>> 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 > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
