<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 class="im"><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.<u></u>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. 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 class="HOEnZb"><div class="h5"><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><br></div></div></div><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 class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Alp.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Alp.<br>
<br>
<br>
<br>
<br>
On 15/11/2013 22:45, Richard Smith wrote:<br>
<br>
Author: rsmith<br>
Date: Fri Nov 15 16:45:29 2013<br>
New Revision: 194869<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=194869&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=194869&view=rev</a><br>
Log:<br>
PR8455: Handle an attribute between a goto label and a<br>
variable declaration per<br>
the GNU documentation: the attribute only appertains to the<br>
label if it is<br>
followed by a semicolon. Based on a patch by Aaron Ballman!<br>
<br>
Modified:<br>
cfe/trunk/lib/Parse/ParseStmt.<u></u>cpp<br>
cfe/trunk/test/Misc/ast-dump-<u></u>attr.cpp<br>
cfe/trunk/test/Sema/warn-<u></u>unused-label.c<br>
cfe/trunk/test/SemaCXX/warn-<u></u>unused-variables.cpp<br>
<br>
Modified: cfe/trunk/lib/Parse/ParseStmt.<u></u>cpp<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseStmt.cpp?rev=194869&r1=194868&r2=194869&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/cfe/trunk/lib/Parse/<u></u>ParseStmt.cpp?rev=194869&r1=<u></u>194868&r2=194869&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- cfe/trunk/lib/Parse/ParseStmt.<u></u>cpp (original)<br>
+++ cfe/trunk/lib/Parse/ParseStmt.<u></u>cpp Fri Nov 15 16:45:29 2013<br>
@@ -516,11 +516,40 @@ StmtResult Parser::ParseLabeledStatement<br>
// identifier ':' statement<br>
SourceLocation ColonLoc = ConsumeToken();<br>
- // Read label attributes, if present. attrs will contain<br>
both C++11 and GNU<br>
- // attributes (if present) after this point.<br>
- MaybeParseGNUAttributes(attrs)<u></u>;<br>
+ // Read label attributes, if present.<br>
+ StmtResult SubStmt;<br>
+ if (Tok.is(tok::kw___attribute)) {<br>
+ ParsedAttributesWithRange TempAttrs(AttrFactory);<br>
+ ParseGNUAttributes(TempAttrs);<br>
+<br>
+ // In C++, GNU attributes only apply to the label if they<br>
are followed by a<br>
+ // semicolon, to disambiguate label attributes from<br>
attributes on a labeled<br>
+ // declaration.<br>
+ //<br>
+ // This doesn't quite match what GCC does; if the<br>
attribute list is empty<br>
+ // and followed by a semicolon, GCC will reject (it<br>
appears to parse the<br>
+ // attributes as part of a statement in that case). That<br>
looks like a bug.<br>
+ if (!getLangOpts().CPlusPlus || Tok.is(tok::semi))<br>
+ attrs.takeAllFrom(TempAttrs);<br>
+ else if (isDeclarationStatement()) {<br>
+ StmtVector Stmts;<br>
+ // FIXME: We should do this whether or not we have a<br>
declaration<br>
+ // statement, but that doesn't work correctly (because<br>
ProhibitAttributes<br>
+ // can't handle GNU attributes), so only call it in the<br>
one case where<br>
+ // GNU attributes are allowed.<br>
+ SubStmt = ParseStatementOrDeclarationAft<u></u>erAttributes(<br>
+ Stmts, /*OnlyStmts*/ true, 0, TempAttrs);<br>
+ if (!TempAttrs.empty() && !SubStmt.isInvalid())<br>
+ SubStmt = Actions.ProcessStmtAttributes(<br>
+ SubStmt.get(), TempAttrs.getList(), TempAttrs.Range);<br>
+ } else {<br>
+ Diag(Tok, diag::err_expected_semi_after) <<<br>
"__attribute__";<br>
+ }<br>
+ }<br>
- StmtResult SubStmt(ParseStatement());<br>
+ // If we've not parsed a statement yet, parse one now.<br>
+ if (!SubStmt.isInvalid() && !SubStmt.isUsable())<br>
+ SubStmt = ParseStatement();<br>
// Broken substmt shouldn't prevent the label from being<br>
added to the AST.<br>
if (SubStmt.isInvalid())<br>
@@ -557,7 +586,7 @@ StmtResult Parser::ParseCaseStatement(bo<br>
// out of stack space in our recursive descent parser. As<br>
a special case,<br>
// flatten this recursion into an iterative loop. This is<br>
complex and gross,<br>
// but all the grossness is constrained to<br>
ParseCaseStatement (and some<br>
- // wierdness in the actions), so this is just local<br>
grossness :).<br>
+ // weirdness in the actions), so this is just local<br>
grossness :).<br>
// TopLevelCase - This is the highest level we have<br>
parsed. 'case 1' in the<br>
// example above.<br>
<br>
Modified: cfe/trunk/test/Misc/ast-dump-<u></u>attr.cpp<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/ast-dump-attr.cpp?rev=194869&r1=194868&r2=194869&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/cfe/trunk/test/Misc/<u></u>ast-dump-attr.cpp?rev=194869&<u></u>r1=194868&r2=194869&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- cfe/trunk/test/Misc/ast-dump-<u></u>attr.cpp (original)<br>
+++ cfe/trunk/test/Misc/ast-dump-<u></u>attr.cpp Fri Nov 15 16:45:29 2013<br>
@@ -95,3 +95,19 @@ void *TestVariadicUnsigned1(int) __attri<br>
void *TestVariadicUnsigned2(int, int)<br>
__attribute__((alloc_size(1,2)<u></u>));<br>
// CHECK: FunctionDecl{{.*}}<u></u>TestVariadicUnsigned2<br>
// CHECK: AllocSizeAttr{{.*}} 0 1<br>
+<br>
+void TestLabel() {<br>
+L: __attribute__((unused)) int i;<br>
+// CHECK: LabelStmt{{.*}}'L'<br>
+// CHECK: VarDecl{{.*}}i 'int'<br>
+// CHECK-NEXT: UnusedAttr{{.*}}<br>
+<br>
+M: __attribute(()) int j;<br>
+// CHECK: LabelStmt {{.*}} 'M'<br>
+// CHECK-NEXT: DeclStmt<br>
+// CHECK-NEXT: VarDecl {{.*}} j 'int'<br>
+<br>
+N: __attribute(()) ;<br>
+// CHECK: LabelStmt {{.*}} 'N'<br>
+// CHECK-NEXT: NullStmt<br>
+}<br>
<br>
Modified: cfe/trunk/test/Sema/warn-<u></u>unused-label.c<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-unused-label.c?rev=194869&r1=194868&r2=194869&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/cfe/trunk/test/Sema/<u></u>warn-unused-label.c?rev=<u></u>194869&r1=194868&r2=194869&<u></u>view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- cfe/trunk/test/Sema/warn-<u></u>unused-label.c (original)<br>
+++ cfe/trunk/test/Sema/warn-<u></u>unused-label.c Fri Nov 15<br>
16:45:29 2013<br>
@@ -9,3 +9,7 @@ void f() {<br>
goto d;<br>
return;<br>
}<br>
+<br>
+void PR8455() {<br>
+ L: __attribute__((unused)) return; // ok, no semicolon required<br>
+}<br>
<br>
Modified: cfe/trunk/test/SemaCXX/warn-<u></u>unused-variables.cpp<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-unused-variables.cpp?rev=194869&r1=194868&r2=194869&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/cfe/trunk/test/<u></u>SemaCXX/warn-unused-variables.<u></u>cpp?rev=194869&r1=194868&r2=<u></u>194869&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- cfe/trunk/test/SemaCXX/warn-<u></u>unused-variables.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/warn-<u></u>unused-variables.cpp Fri Nov<br>
15 16:45:29 2013<br>
@@ -1,4 +1,4 @@<br>
-// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -verify %s<br>
+// RUN: %clang_cc1 -fsyntax-only -Wunused-variable<br>
-Wunused-label -verify %s<br>
template<typename T> void f() {<br>
T t;<br>
t = 17;<br>
@@ -128,3 +128,26 @@ namespace ctor_with_cleanups {<br>
}<br>
#include "Inputs/warn-unused-variables.<u></u>h"<br>
+<br>
+namespace PR8455 {<br>
+ void f() {<br>
+ A: // expected-warning {{unused label 'A'}}<br>
+ __attribute__((unused)) int i; // attribute applies to<br>
variable<br>
+ B: // attribute applies to label<br>
+ __attribute__((unused)); int j; // expected-warning<br>
{{unused variable 'j'}}<br>
+ }<br>
+<br>
+ void g() {<br>
+ C: // unused label 'C' will not appear here because an<br>
error occurs<br>
+ __attribute__((unused))<br>
+ #pragma weak unused_local_static // expected-error<br>
{{expected ';' after __attribute__}}<br>
+ ;<br>
+ }<br>
+<br>
+ void h() {<br>
+ D: // expected-warning {{unused label 'D'}}<br>
+ #pragma weak unused_local_static<br>
+ __attribute__((unused)) // expected-warning<br>
{{declaration does not declare anything}}<br>
+ ;<br>
+ }<br>
+}<br>
<br>
<br>
______________________________<u></u>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a> <mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.<u></u>edu</a>><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
<br>
<br>
-- <a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</div></div></blockquote></div><br></div></div>