Page MenuHomePhabricator

[Clangd] Fixed SelectionTree bug for macros

Authored by SureYeaah on Jul 8 2019, 6:29 AM.



Fixed SelectionTree bug for macros

  • Fixed SelectionTree claimRange for macros and template instantiations
  • Fixed SelectionTree unit tests

Event Timeline

SureYeaah created this revision.Jul 8 2019, 6:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2019, 6:29 AM
SureYeaah updated this revision to Diff 208387.Jul 8 2019, 6:31 AM

Removed debugging code

SureYeaah updated this revision to Diff 208389.Jul 8 2019, 6:35 AM

Removed extra includes

sammccall added inline comments.Jul 8 2019, 10:17 AM
245 ↗(On Diff #210783)

the new wording seems less clear to me

447 ↗(On Diff #208389)

It looks like your editor might be set up to format the whole file, rather than just changed lines (git clang-format will also do this).

Please avoid this in general as it messes up blame history.
No need to go back and revert everything though.

655 ↗(On Diff #208389)

This is Lexer::getLocForEndOfToken(Loc, 0, SM, LangOpts), I think

663 ↗(On Diff #208389)

Returns a half-open CharSourceRange.
(Because closed character CharSourceRanges are also a thing...)

683 ↗(On Diff #208389)

nit: do the conversion from the previous line here, and write the loop as a regular while loop?

204 ↗(On Diff #208389)

This is closely related to toHalfOpenFileRange and should be moved up there.

Actually... is this just a less-buggy replacement for toHalfOpenFileRange? If so, we should give it that name. It'd be nice to add a few unit tests (and verify that they fail with the old version of the function).

205 ↗(On Diff #208389)

"end of the last token" is confusing: does it point to the last char, or one-past-last?

The latter seems nicest and is easily described by the existing "half-open" name

SureYeaah updated this revision to Diff 209199.Jul 11 2019, 6:16 AM
SureYeaah marked 6 inline comments as done.

Changed claimRange to use toHalfOpenFileRange

SureYeaah updated this revision to Diff 209229.Jul 11 2019, 8:26 AM

Changed a breaking test in ExtractVariable

SureYeaah updated this revision to Diff 209506.Jul 12 2019, 9:11 AM

Added a test case

Just doc nits I think.

This eliminates a semi-fast path (that uses approximate token matching to avoid running the lexer in claimRange sometimes) but I think we can live without it.

242 ↗(On Diff #210783)

I'm not sure why the FIXME is on this function, it can't be fixed here.

If you're not sure where to put it, I'd suggest putting the FIXME on the unit test that shows the wrong behavior.

257–258 ↗(On Diff #210783)

The rest of this function talks about cheap vs precise checks etc - you've removed the cheap path (which is OK to improve correctness, I think), so the comments need to be rewritten

SureYeaah updated this revision to Diff 210783.Jul 19 2019, 2:33 AM
SureYeaah marked 2 inline comments as done.

Updated comments

sammccall accepted this revision.Jul 19 2019, 4:06 AM
sammccall added inline comments.
399 ↗(On Diff #210783)

can you uncomment this case, and assert the actual behavior? The FIXME should say what it should be instead.

This revision is now accepted and ready to land.Jul 19 2019, 4:06 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2019, 4:40 AM