On Mon, Sep 10, 2012 at 6:41 PM, Magee, Josh <span dir="ltr"><<a href="mailto:Joshua.Magee@am.sony.com" target="_blank">Joshua.Magee@am.sony.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Attached is a new patch that (a) fixes the assertion when emitting the<br>
warning dialogue in C++98 mode, and (b) implements C++11<br>
[class.friend]p3 (PR9834).<br></blockquote><div><br></div><div>Thanks!</div><div><br></div><div><div style="background-color:rgb(255,255,255)"><div><font color="#222222" face="arial, sans-serif">> --- a/include/clang/Basic/DiagnosticSemaKinds.td</font></div>
<div><font color="#222222" face="arial, sans-serif">> +++ b/include/clang/Basic/DiagnosticSemaKinds.td</font></div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:13px"><br></div></div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)">
For your future consideration: Convention on these lists is to use -p0 patches instead of -p1 patches. You can configure <span class="il" style="background-color:rgb(255,255,204)">git</span> with '[diff] noprefix = true' for this.</div>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
For (a) I removed FriendLoc from the highlight range. As noted, the<br>
caret is pointing to FriendLoc anyway.<br></blockquote><div> </div><div>> +++ b/lib/Sema/SemaDeclCXX.cpp</div><div><div>> @@ -9879,7 +9880,7 @@ FriendDecl *Sema::CheckFriendTypeDecl(SourceLocation Loc,</div><div>> diag::warn_cxx98_compat_nonclass_type_friend :</div>
<div>> diag::ext_nonclass_type_friend)</div><div>> << T</div></div><div>> - << SourceRange(FriendLoc, TypeRange.getEnd());</div><div><div>> + << SourceRange(TypeRange.getBegin(), TypeRange.getEnd());</div>
</div><div><br></div><div>"<< TypeRange" should work just as well here (2x).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
For (b): During parsing, I flagged whether or not the friend keyword<br>
appears first in friend declarations. Unfortunately, since the standard<br>
only requires friend to be the first token in non-function friend<br>
declarations, it isn't possible to generate an error at this point.<br>
Instead, in Semantic Analysis, generate an error if the FriendFirst flag<br>
is not set when checking a friend type (i.e., non-function) declaration.<br></blockquote><div><br></div><div><div>> + if (DSContext == DSC_class) {</div><div>> + bool FriendFirst = !DS.hasTypeSpecifier();</div>
<div>> + isInvalid = DS.SetFriendSpec(Loc, PrevSpec, DiagID, FriendFirst);</div></div><div><br></div><div>hasTypeSpecifier doesn't check for cv-qualifiers (and also some Altivec oddball cases like "__vector friend __pixel;"). IIRC, even DeclSpec::isEmpty misses the latter case. You could instead check for a leading 'friend' in ParseDeclarationSpecifiers, or by comparing FriendLoc against Range.getBeginLoc().</div>
<div> </div><div>> +++ b/test/SemaCXX/friend-diagnostic-crash.cpp</div><div><br></div><div>Please add this test to test/SemaCXX/friend.cpp instead. Extra test files cost more than extra code in existing tests. test/CXX/class/class.friend/p3.cpp is fine, though.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks,<br>
Josh<br>
<div><br>
<br>
At 1346342316 seconds past the Epoch, Richard Smith wrote:<br>
</div><div><div>> I wonder if we should even get as far as producing that diagnostic in this<br>
> case. The code is ill-formed: for a non-function friend declaration,<br>
> 'friend' must be the first token in the declaration (see C++11<br>
> [class.friend]p3).<br>
><br>
> + // Check whether the "friend" specifier is before or after the<br>
> + // type specifiers to determine the highlighted range.<br>
> + SourceRange HighlightRange = TypeRange.getEnd() < FriendLoc ?<br>
> + SourceRange(TypeRange.getBegin(),<br>
> PP.getLocForEndOfToken(FriendLoc)) :<br>
> + SourceRange(FriendLoc, TypeRange.getEnd());<br>
><br>
> operator< on SourceLocation does not do what you expect here, and this<br>
> doesn't form the correct range in cases where the 'friend' keyword is<br>
> neither at the start nor at the end of the range, for instance "unsigned<br>
> friend int;". How about just not including FriendLoc in the highlighted<br>
> range at all? We're already pointing the caret at it.<br>
><br>
> On Wed, Aug 29, 2012 at 5:50 PM, Magee, Josh <<a href="mailto:Joshua.Magee@am.sony.com" target="_blank">Joshua.Magee@am.sony.com</a>>wrote:<br>
><br>
> > Hi,<br>
> ><br>
> > Attached is a patch to fix an assertion that occurs when printing<br>
> > diagnostic<br>
> > warnings for non-class friend usage.<br>
> ><br>
> > Example:<br>
> > This case prints fine:<br>
> > struct { friend int; } a;<br>
> > warning: non-class friend type 'int' is a C++11 extension<br>
> > [-Wc++11-extensions]<br>
> > friend int;<br>
> > ^~~~~~~~~~<br>
> ><br>
> > This case causes an assertion:<br>
> > struct {int friend; } a;<br>
> > warning: non-class friend type 'int' is a C++11 extension<br>
> > [-Wc++11-extensions]<br>
> > ...<br>
> > Assertion `StartColNo <= EndColNo && "Invalid range!"' failed.<br>
> ><br>
> > The problem is that when printing the diagnostic the possibility that the<br>
> > friend specifier may appear after the type specifier is not considered.<br>
> > The<br>
> > fix is to check whether the friend specifier appears after the type and<br>
> > adjust<br>
> > the highlight range accordingly.<br>
> ><br>
> ><br>
> > Can someone review and, if everything is OK, commit?<br>
> ><br>
> > Thanks!<br>
> > Josh<br>
> > _______________________________________________<br>
> > cfe-commits mailing list<br>
> > <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
> ><br>
> ></div></div></blockquote></div><br>