[cfe-commits] [PATCH] Fix bad highlight range when diagnosing non-class friend usage

[Magee, Josh]

At 1347336906 seconds past the Epoch, Richard Smith wrote:
> On Mon, Sep 10, 2012 at 6:41 PM, Magee, Josh <Joshua.Magee at am.sony.com>wrote:
> > --- a/include/clang/Basic/DiagnosticSemaKinds.td
> > +++ b/include/clang/Basic/DiagnosticSemaKinds.td
> 
> For your future consideration: Convention on these lists is to use -p0
> patches instead of -p1 patches. You can configure git with '[diff] noprefix
> = true' for this.
Thanks for the tip, I updated my git config.
 
> > +++ b/lib/Sema/SemaDeclCXX.cpp
> > @@ -9879,7 +9880,7 @@ FriendDecl
> *Sema::CheckFriendTypeDecl(SourceLocation Loc,
> >               diag::warn_cxx98_compat_nonclass_type_friend :
> >               diag::ext_nonclass_type_friend)
> >          << T
> > -        << SourceRange(FriendLoc, TypeRange.getEnd());
> > +        << SourceRange(TypeRange.getBegin(), TypeRange.getEnd());
> 
> "<< TypeRange" should work just as well here (2x).
Done.

> > +      if (DSContext == DSC_class) {
> > +        bool FriendFirst = !DS.hasTypeSpecifier();
> > +        isInvalid = DS.SetFriendSpec(Loc, PrevSpec, DiagID, FriendFirst);
> 
> 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().
Okay, comparing against FriendLoc is much simpler than what I was doing
and it handles cv-qualifiers and the Altivec __vector cases.

 
> > +++ b/test/SemaCXX/friend-diagnostic-crash.cpp
> 
> 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.
> 
I removed the test from the latest patch entirely for now.  The purpose
of the test was to make sure that there wasn't a crash when printing out the
highlight range.  Adding it to test/SemaCXX/friend.cpp doesn't catch
this since the test is run with -verify which avoids the crash entirely.
I could add an extra run line to friend.cpp to run again without
-verify, but I don't know if that is preferable to having a separate
test or not.  Maybe there is a better place for the test entirely?

test/SemaCXX/cxx98-compat.cpp already handles checking for the
warning "non-class friend type 'x' is a C++11 extension".



Thanks,
Josh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: friends-first-update.diff
Type: application/octet-stream
Size: 3206 bytes
Desc: friends-first-update.diff
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120913/df9ea18a/attachment.obj>