This is an archive of the discontinued LLVM Phabricator instance.

Fix toHalfOpenFileRange assertion fail
ClosedPublic

Authored by SureYeaah on Aug 5 2019, 9:14 AM.

Event Timeline

SureYeaah created this revision.Aug 5 2019, 9:14 AM
SureYeaah updated this revision to Diff 213583.Aug 6 2019, 5:00 AM

Implemented function to get expansion range such that ends are both in the same file id.

sammccall added inline comments.Aug 6 2019, 6:30 AM
clang-tools-extra/clangd/SourceCode.cpp
307

nit: getExpansionTokenRangeInSameFile?

315

Having a little trouble parsing this comment...

"Record the stack of expansion locations for the beginning, keyed by FileID"?

316

DenseMap<FileID, SourceRange>

Why are the values SourceRange here and not just SourceLocation?

318

Is there a way to write this loop that better captures the intent? while(1) with a break in the middle is hard for readers to understand at a glance.

I think it's just

for (SourceLocation Begin = ExpansionRange.getBegin();
       Begin.isValid();
       Begin = Begin.isFileId() ? SourceLocation() : SM.getImmediateExpansionRange(Begin).getBegin())
  BeginExpansions[SM.getFileID(Begin)] = Begin;
323

this call to toTokenRange seems unneccesary, that just adjusts End, but we only care about Start

328

again, I think by treating these as ranges rather than points you're making it harder than it needs to be. Traversing up the expansion tree isn't going to invert the order (though spelling might)

333

isn't this always SourceRange(It->second.getBegin(), EndExpansion.getEnd())?

357

nit: as with comments, avoid adding messages to the assertion unless it adds something to the code

SureYeaah updated this revision to Diff 213599.Aug 6 2019, 7:03 AM
SureYeaah marked 8 inline comments as done.

Addressed review comments

sammccall added inline comments.Aug 6 2019, 7:53 AM
clang-tools-extra/clangd/SourceCode.cpp
316

meant to say here: I think FileID is a legitimate key in a DenseMap, no need for unsigned

(the DenseMapInfo<clang::FileID> specialization exists, which should make it work)

SureYeaah updated this revision to Diff 213614.Aug 6 2019, 7:55 AM

use fileID instead of hash in map

SureYeaah marked an inline comment as done.Aug 6 2019, 8:10 AM
sammccall accepted this revision.Aug 6 2019, 9:38 AM

Sorry, should have accepted this in the last round. Great stuff, thank you!

This revision is now accepted and ready to land.Aug 6 2019, 9:38 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2019, 10:02 AM