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

[Richard Smith]

On Mon, Sep 10, 2012 at 6:41 PM, Magee, Josh <Joshua.Magee at am.sony.com>wrote:

> Attached is a new patch that (a) fixes the assertion when emitting the
> warning dialogue in C++98 mode, and (b) implements C++11
> [class.friend]p3 (PR9834).
>

Thanks!

> --- 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.


> For (a) I removed FriendLoc from the highlight range.  As noted, the
> caret is pointing to FriendLoc anyway.
>

> +++ 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).


> For (b): During parsing, I flagged whether or not the friend keyword
> appears first in friend declarations.  Unfortunately, since the standard
> only requires friend to be the first token in non-function friend
> declarations, it isn't possible to generate an error at this point.
> Instead, in Semantic Analysis, generate an error if the FriendFirst flag
> is not set when checking a friend type (i.e., non-function) declaration.
>

> +      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().

> +++ 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.

Thanks,
> Josh
>
>
> At 1346342316 seconds past the Epoch, Richard Smith wrote:
> > I wonder if we should even get as far as producing that diagnostic in
> this
> > case. The code is ill-formed: for a non-function friend declaration,
> > 'friend' must be the first token in the declaration (see C++11
> > [class.friend]p3).
> >
> > +  // Check whether the "friend" specifier is before or after the
> > +  // type specifiers to determine the highlighted range.
> > +  SourceRange HighlightRange = TypeRange.getEnd() < FriendLoc ?
> > +         SourceRange(TypeRange.getBegin(),
> > PP.getLocForEndOfToken(FriendLoc)) :
> > +         SourceRange(FriendLoc, TypeRange.getEnd());
> >
> > operator< on SourceLocation does not do what you expect here, and this
> > doesn't form the correct range in cases where the 'friend' keyword is
> > neither at the start nor at the end of the range, for instance "unsigned
> > friend int;". How about just not including FriendLoc in the highlighted
> > range at all? We're already pointing the caret at it.
> >
> > On Wed, Aug 29, 2012 at 5:50 PM, Magee, Josh <Joshua.Magee at am.sony.com
> >wrote:
> >
> > > Hi,
> > >
> > > Attached is a patch to fix an assertion that occurs when printing
> > > diagnostic
> > > warnings for non-class friend usage.
> > >
> > > Example:
> > > This case prints fine:
> > > struct { friend int; } a;
> > >   warning: non-class friend type 'int' is a C++11 extension
> > > [-Wc++11-extensions]
> > >   friend int;
> > >   ^~~~~~~~~~
> > >
> > > This case causes an assertion:
> > > struct {int friend; } a;
> > >   warning: non-class friend type 'int' is a C++11 extension
> > > [-Wc++11-extensions]
> > > ...
> > > Assertion `StartColNo <= EndColNo && "Invalid range!"' failed.
> > >
> > > The problem is that when printing the diagnostic the possibility that
> the
> > > friend specifier may appear after the type specifier is not considered.
> > >  The
> > > fix is to check whether the friend specifier appears after the type and
> > > adjust
> > > the highlight range accordingly.
> > >
> > >
> > > Can someone review and, if everything is OK, commit?
> > >
> > > Thanks!
> > > Josh
> > > _______________________________________________
> > > cfe-commits mailing list
> > > cfe-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120910/5393c985/attachment.html>