<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Nov 15, 2013 at 8:31 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div class="im">
<br>
<div>On 16/11/2013 01:32, Richard Smith
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">On Fri, Nov 15, 2013 at 4:07 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span>
wrote:<br>
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
On 16/11/2013 00:00, Alp Toker wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 15/11/2013 23:42, Richard Smith wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Fri, Nov 15, 2013 at 3:33 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>
<mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>>
wrote:<br>
<br>
Hello Richard,<br>
<br>
For the record I had a patch up for review to
fix this back from<br>
2010, not sure if it ever reached the list:<br>
<br>
<a href="http://llvm.org/bugs/show_bug.cgi?id=8455" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=8455</a><br>
<br>
Just to compare notes, my approach was to use a<br>
TentativeParsingAction to handle some of the
corner cases.<br>
<br>
<br>
Aaron's original patch went in that direction, but
got reworked to avoid the cost of a tentative parse.<br>
</blockquote>
<br>
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.<br>
</blockquote>
<br>
</div>
In fact, this applies not just to Microsoft-style
attributes but also Objective-C messages using square
brackets.<br>
<br>
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.<br>
<br>
Have you actually tested corner cases like this?</blockquote>
<div><br>
</div>
<div>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.</div>
</div>
</div>
</div>
</blockquote>
<br></div>
Come on Richard, you know there's a problem here because there's a
big FIXME added it in the patch you landed:<div class="im"><br>
<br>
<code>+ else if (isDeclarationStatement()) {</code><code><br>
</code><code>+ StmtVector Stmts;</code><code><br>
</code><b><code>+ // FIXME: We should do this whether or not we
have a declaration</code></b><b><code><br>
</code></b><code>+ // statement, but that doesn't work
correctly (because ProhibitAttributes</code><br>
<br></div>
It's obvious from inspection that:<br>
<br>
<ol>
<li>Non-declaration statements can end up here.</li>
<li>isDeclarationStatement() isn't enough to detect everything a
GNU label can be applied to in this context.</li></ol></div></blockquote><div><br></div><div>Got a counterexample?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><ol>
<li>A tentative parse will yield the correct answer every time.</li></ol></div></blockquote><div>The bug is that ProhibitAttributes doesn't work, so we do the 'isDeclarationStatement' dance unnecessarily.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
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.</div></blockquote><div><br></div><div>We avoid tentative parses where possible, for performance reasons.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div class="im">
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>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.</div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
<div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Judging that PR8455 was closed shortly after the
commit, I'm guessing you or Aaron knew my patch
existed beforehand.<br>
<br>
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.<br>
</blockquote>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>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...</div>
</div>
</div>
</div>
</blockquote>
<br></div>
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.<br>
<br>
(But Alp from three years ago says thanks for the tip!)<div class="im"><br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>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).</div>
</div>
</div>
</div>
</blockquote>
<br></div>
Not demanding anything, just pointing out that your patch is wrong.<br></div></blockquote><div><br></div><div>You've not actually pointed that out yet. I'm still waiting for a testcase.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
Cheers,<br>
Alp.<div class="im"><br>
<br>
<pre cols="72">--
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a>
the browser experts
</pre>
</div></div>
</blockquote></div><br></div></div>