This is an archive of the discontinued LLVM Phabricator instance.

Correct invalid end location in diagnostics for some identifiers.
AbandonedPublic

Authored by erikjv on Jun 7 2016, 7:38 AM.

Details

Summary

Declaration names in DeclSpec had only their start set to a valid
location, so when the type specifier was missing, only the carret would
be shown at the first character of the name of a member declaration. Now
the whole identifier is underlined, which is useful for IDEs using
libclang.

Also, when the lookup for an identifier-expression failed, the end
location for that identifier was invalid.

Fixes PR25374.

Diff Detail

Event Timeline

erikjv updated this revision to Diff 59886.Jun 7 2016, 7:38 AM
erikjv retitled this revision from to Correct invalid end location in diagnostics for some identifiers..
erikjv updated this object.
erikjv added reviewers: bkramer, klimek.
erikjv added a subscriber: cfe-commits.
klimek added inline comments.Jun 28 2016, 7:36 AM
lib/Sema/SemaType.cpp
1339 ↗(On Diff #59886)

Do you know in which cases we get source ranges that are half valid?

erikjv added a comment.Jul 5 2016, 7:02 AM

I'll add tests in the next diff, but I'd like to know if these changes are ok.

lib/Sema/SemaType.cpp
1339 ↗(On Diff #59886)

In Parser::ParseTypeofSpecifier the 'cast range' for a typeof(...) is parsed, but is found to be invalid. Then it explicitly sets the DeclSpec's end location to an invalid location.

klimek added inline comments.
lib/Sema/SemaType.cpp
1339 ↗(On Diff #59886)

Ok, in this case, we'll need Richard to verify whether he intended half-valid source locations something that users need to deal with.

rsmith added inline comments.Oct 26 2016, 12:50 PM
lib/Sema/SemaExpr.cpp
2046–2047

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.

lib/Sema/SemaType.cpp
1339 ↗(On Diff #59886)

The code in ParseTypeofSpecifier does this:

if (CastRange.getEnd().isInvalid())
  // FIXME: Not accurate, the range gets one token more than it should.
  DS.SetRangeEnd(Tok.getLocation());
else
  DS.SetRangeEnd(CastRange.getEnd());

This should always set the end of the range to a valid location. Generally, I think we should avoid half-valid ranges rather than trying to cope with them downstream.

erikjv added inline comments.Oct 26 2016, 1:05 PM
lib/Sema/SemaExpr.cpp
2046–2047

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.

klimek edited edge metadata.Oct 27 2016, 12:38 AM

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.

Aren't we mostly doing token positions? It's basically always wrong to point at a single character, right?

erikjv updated this revision to Diff 80735.Dec 8 2016, 2:41 AM
erikjv edited edge metadata.
rsmith added inline comments.Feb 7 2017, 2:11 PM
lib/Parse/ParseDecl.cpp
2702

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?

lib/Sema/SemaExpr.cpp
2046–2047

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.

erikjv abandoned this revision.May 30 2017, 5:01 AM