<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, May 23, 2014 at 10:47 AM, Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank">sepavloff@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
2014-05-23 6:43 GMT+07:00 Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span>:<div class=""><br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>--- lib/Parse/ParseDecl.cpp</div><div>
+++ lib/Parse/ParseDecl.cpp</div><div>@@ -2706,7 +2706,7 @@</div>
<div> case tok::identifier: {</div><div> // In C++, check to see if this is a scope specifier like foo::bar::, if</div>
<div> // so handle it as such. This is important for ctor parsing.</div><div>- if (getLangOpts().CPlusPlus) {</div><div>+ if (getLangOpts().CPlusPlus && !DS.hasTypeSpecifier()) {</div><div><br></div>
<div>Please instead swap over this 'if' and the next one.</div><div class="gmail_extra"><br></div></div></blockquote><div><br></div></div><div>Removed unneded check. </div><div class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"></div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">@@ -4508,8 +4508,10 @@</div><div class="gmail_extra">
// Member pointers get special handling, since there's no place for the</div><div class="gmail_extra"> // scope spec in the generic path below.</div><div class="gmail_extra"> if (getLangOpts().CPlusPlus &&</div>
<div class="gmail_extra">- (Tok.is(tok::coloncolon) || Tok.is(tok::identifier) ||</div><div class="gmail_extra">- Tok.is(tok::annot_cxxscope))) {</div><div class="gmail_extra">+ (Tok.is(tok::coloncolon) ||</div>
<div class="gmail_extra">+ (Tok.is(tok::identifier) && (NextToken().is(tok::coloncolon) ||</div><div class="gmail_extra">+ NextToken().is(tok::less))) ||</div><div class="gmail_extra">
+ Tok.is(tok::annot_cxxscope))) {</div><div><br></div><div>I don't think this is quite sufficient. If we have:</div><div><br></div><div> struct A { enum E {}; };</div><div> struct B {</div><div> static const int N = 3;</div>
<div> A::E : N;</div><div> };</div><div><br></div><div>... I think we might treat the : as a :: typo. Instead, drop a ColonProtectionRAII object into this 'if', with a comment saying you're doing it for bitfields, if you're in a member context.</div>
</div></div></blockquote><div><br></div></div><div>Not sure if I undestand correctly. This piece of code is correcly parsed as unnamed bit field of enum type. Testcase lacked for tests on unnamed bitfields, they are added now.</div>
</div></div></div></blockquote><div><br></div><div>You may need to try a bit harder to get this to misparse, but I think it will. The point is that we're not protecting colons when we parse the scope specifier, and the check:</div>
<div><br></div><div><div>- (Tok.is(tok::coloncolon) || Tok.is(tok::identifier) ||</div><div>- Tok.is(tok::annot_cxxscope))) {</div><div>+ (Tok.is(tok::coloncolon) ||</div><div>+ (Tok.is(tok::identifier) && (NextToken().is(tok::coloncolon) ||</div>
<div>+ NextToken().is(tok::less))) ||</div><div>+ Tok.is(tok::annot_cxxscope))) {</div></div><div><br></div><div>only protects a colon at the start, not a colon *after* the scope. Here's another attempt to get this to fail:</div>
<div><br></div><div> const int m = 4; struct A { struct n { int m; }; int A::n : m; };<br></div><div><br></div><div>This code is valid with -fms-extensions, but I think you might correct the ':' to a '::' here.</div>
<div><br></div><div>What I'm suggesting is that you revert the above change and add in colon-protection around the parsing of the scope specifier. Other than that, this patch looks great!</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div><div><div class="gmail_quote">On Fri, May 9, 2014 at 4:17 AM, Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank">sepavloff@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_quote">Hi Richard,</div><div class="gmail_quote"><br></div><div class="gmail_quote">Could you please review this fix?</div><div class="gmail_quote">Thank you.</div><div class="gmail_quote">
<br></div><div class="gmail_quote">--Serge</div><div class="gmail_quote"><br>Recognize additional cases, when '::' is mistyped as ':' and provide<br>
descriptive diagnostics for them.<br>
This is a fix to RP18587 - colons have too much protection in member-declarations<br>
<br>
<a href="http://reviews.llvm.org/D3653" target="_blank">http://reviews.llvm.org/D3653</a><br>
<br>
Files:<br>
lib/Parse/ParseDecl.cpp<br>
lib/Parse/ParseDeclCXX.cpp<br>
test/SemaCXX/nested-name-spec.cpp<br>
</div><div><br></div>
</div>
</blockquote></div><br></div></div></div></div>
</blockquote></div></div><span class=""><font color="#888888"><br><br clear="all"><div><br></div>-- <br>Thanks,<br>--Serge<br>
</font></span></div></div>
</blockquote></div><br></div></div>