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

[Magee, Josh]

Thank you for taking time to review, see my comments below.

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

I wondered the same thing.  Recent versions of gcc (4.7+) accept these
ill-formed declarations in C++11 mode without warning.  Of course just
because gcc accepts it doesn't mean clang necessarily should,
particularly in cases where gcc is not fully following the standard.

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

Not including FriendLoc in the highlighted range certainly seems
reasonable, and the diagnostic looks perfectly fine for 'friend int;'
and 'int friend;'.  In the case of 'unsigned friend int', however it is
not ideal:

warning: non-class friend type 'unsigned int' is a C++11 extension [-Wc++11-extensions]
        unsigned friend int;
        ~~~~~~~~ ^

Maybe this is acceptable, but I would think this is preferable:
        unsigned friend int;
        ~~~~~~~~ ^      ~~~~

Highlighting the entire range solves this, but as you pointed out the
proposed solution didn't handle this case properly.

Let me think this over some more and I'll get back to you.


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