This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include
ClosedPublic

Authored by sammccall on Aug 22 2019, 6:53 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Aug 22 2019, 6:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2019, 6:53 AM
sammccall updated this revision to Diff 216609.Aug 22 2019, 7:00 AM

kick phabricator to try to get it to mail

sammccall marked 4 inline comments as done.Aug 22 2019, 7:04 AM
sammccall added inline comments.
clang-tools-extra/clangd/SourceCode.cpp
318 ↗(On Diff #216609)

I dropped this function because it's just SM.isSpelledInSameFile

338 ↗(On Diff #216609)

this is an unrelated bug fix: E1 < E2, std::min() etc don't do anything sensible for SourceLocation

358 ↗(On Diff #216609)

This is half of the real fix: once we've hit the top of the macro tree, we should walk up the #include stack in case we find a common ancestor there.

443 ↗(On Diff #216609)

this is one part of the main fix: the start and endpoint may be in different real files in the presence of #include

SureYeaah accepted this revision.Aug 23 2019, 5:48 AM

LGTM

clang-tools-extra/clangd/SourceCode.cpp
284 ↗(On Diff #216609)

nit: use this as a while condition?

while(Offset-- && Buf[Offset] != '\n')
This revision is now accepted and ready to land.Aug 23 2019, 5:48 AM
sammccall marked an inline comment as done.Aug 27 2019, 1:38 AM
sammccall added inline comments.
clang-tools-extra/clangd/SourceCode.cpp
284 ↗(On Diff #216609)

Ooh, clever... a little too subtle for my taste though :-)
(In particular, allowing the variable to overflow but then ensuring we never read from it...)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2019, 1:48 AM