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