[PATCH] D21075: Correct invalid end location in diagnostics for some identifiers.
Richard Smith via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 7 14:11:27 PST 2017
rsmith added inline comments.
================
Comment at: lib/Parse/ParseDecl.cpp:2702
DS.SetRangeStart(Tok.getLocation());
- DS.SetRangeEnd(SourceLocation());
+ DS.SetRangeEnd(Tok.getLocation());
}
----------------
This doesn't look right: this is a source range containing exactly one token. If we don't have *any* decl-specifiers (such as for a constructor, destructor, or conversion function), this is the wrong range.
How about instead setting the start location to be invalid in `DeclSpec::Finish` if the end location is invalid?
================
Comment at: lib/Sema/SemaExpr.cpp:2061-2062
+ auto Builder = Diag(R.getNameLoc(), diagnostic) << Name;
+ if (Name.isIdentifier())
+ Builder << SourceRange(R.getNameLoc());
return true;
----------------
erikjv wrote:
> rsmith wrote:
> > I'm indifferent on this change: I don't see much point in adding a single-token range when we already point the caret at that token, but I don't see any harm either.
> It's mostly about how much is "underlined". If there is only a caret, that quite often translates into a single character being pointed out, instead of an identifier (i.e. the first character). Hene the extension of the range.
A `^` with no `~`s should be interpreted as indicating a location, not highlighting a single character. We use that pattern in a huge number of diagnostics; it doesn't make sense to do something different here.
I also don't think this is the correct range to underline. If the name comprises multiple tokens (such as `operator int`), this will incorrectly highlight only the first one. A wrong underline seems worse than no underline.
https://reviews.llvm.org/D21075
More information about the cfe-commits
mailing list