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
306

nit: getExpansionTokenRangeInSameFile?

314

Having a little trouble parsing this comment...

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

315

DenseMap<FileID, SourceRange>

Why are the values SourceRange here and not just SourceLocation?

317

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

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

324

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

327

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)

332

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

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
315

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