Please change the name of the first parameter to CheckFriendTypeDecl to be LocStart or similar, and fix the call in SemaTemplateInstantiateDecl to pass in D->getLocStart(), then this LGTM. Do you have commit access, or do you need someone to commit it for you?<br>
<br><div class="gmail_quote">On Wed, Sep 12, 2012 at 6:49 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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
At 1347336906 seconds past the Epoch, Richard Smith wrote:<br>
> On Mon, Sep 10, 2012 at 6:41 PM, Magee, Josh <<a href="mailto:Joshua.Magee@am.sony.com">Joshua.Magee@am.sony.com</a>>wrote:<br>
</div><div class="im">> > --- a/include/clang/Basic/DiagnosticSemaKinds.td<br>
> > +++ b/include/clang/Basic/DiagnosticSemaKinds.td<br>
><br>
> For your future consideration: Convention on these lists is to use -p0<br>
> patches instead of -p1 patches. You can configure git with '[diff] noprefix<br>
> = true' for this.<br>
</div>Thanks for the tip, I updated my git config.<br>
<div class="im"><br>
> > +++ b/lib/Sema/SemaDeclCXX.cpp<br>
> > @@ -9879,7 +9880,7 @@ FriendDecl<br>
> *Sema::CheckFriendTypeDecl(SourceLocation Loc,<br>
> >               diag::warn_cxx98_compat_nonclass_type_friend :<br>
> >               diag::ext_nonclass_type_friend)<br>
> >          << T<br>
> > -        << SourceRange(FriendLoc, TypeRange.getEnd());<br>
> > +        << SourceRange(TypeRange.getBegin(), TypeRange.getEnd());<br>
><br>
> "<< TypeRange" should work just as well here (2x).<br>
</div>Done.<br>
<div class="im"><br>
> > +      if (DSContext == DSC_class) {<br>
> > +        bool FriendFirst = !DS.hasTypeSpecifier();<br>
> > +        isInvalid = DS.SetFriendSpec(Loc, PrevSpec, DiagID, FriendFirst);<br>
><br>
> hasTypeSpecifier doesn't check for cv-qualifiers (and also some Altivec<br>
> oddball cases like "__vector friend __pixel;"). IIRC, even<br>
> DeclSpec::isEmpty misses the latter case. You could instead check for a<br>
> leading 'friend' in ParseDeclarationSpecifiers, or by comparing FriendLoc<br>
> against Range.getBeginLoc().<br>
</div>Okay, comparing against FriendLoc is much simpler than what I was doing<br>
and it handles cv-qualifiers and the Altivec __vector cases.<br>
<div class="im"><br>
<br>
> > +++ b/test/SemaCXX/friend-diagnostic-crash.cpp<br>
><br>
> Please add this test to test/SemaCXX/friend.cpp instead. Extra test files<br>
> cost more than extra code in existing tests.<br>
> test/CXX/class/class.friend/p3.cpp is fine, though.<br>
><br>
</div>I removed the test from the latest patch entirely for now.  The purpose<br>
of the test was to make sure that there wasn't a crash when printing out the<br>
highlight range.  Adding it to test/SemaCXX/friend.cpp doesn't catch<br>
this since the test is run with -verify which avoids the crash entirely.<br>
I could add an extra run line to friend.cpp to run again without<br>
-verify, but I don't know if that is preferable to having a separate<br>
test or not.  Maybe there is a better place for the test entirely?<br>
<br>
test/SemaCXX/cxx98-compat.cpp already handles checking for the<br>
warning "non-class friend type 'x' is a C++11 extension".<br>
<br>
<br>
<br>
Thanks,<br>
Josh</blockquote></div><br>