Page MenuHomePhabricator

[clangd] Include expression in DecltypeTypeLoc sourcerange while building SelectionTree
ClosedPublic

Authored by kadircet on Jan 13 2020, 3:11 AM.

Details

Summary

Currently AST only contains the location for decltype keyword,
therefore we were skipping expressions inside decltype while building selection
tree.

This patch extends source range in such cases to contain the expression as well.
A proper fix would require changes to Sema and DecltypeTypeLoc to contain these
location information.

Fixes https://github.com/clangd/clangd/issues/250.

Diff Detail

Event Timeline

kadircet created this revision.Jan 13 2020, 3:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2020, 3:11 AM

Unit tests: pass. 61742 tests passed, 0 failed and 780 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

sammccall accepted this revision.Jan 13 2020, 10:14 AM

Thanks!

clang-tools-extra/clangd/Selection.cpp
530

Alternately, we could just always return false if it's a DecltypeTypeLoc.

531

I think this comment could be a little clearer on the three ranges, and that the AST one is just incorrect.

e.g.

// DeclTypeTypeLoc::getSourceRange() is incomplete, which would lead to failing
// to descend into the child expression.
// decltype(2+2);
// XXXXXXXXXXXXX <-- correct range
// XXXXXXXX      <-- range reported by getSourceRange()
// XXXXXXXXXXXX  <-- range with this hack
534

nit: parenthesis

This revision is now accepted and ready to land.Jan 13 2020, 10:14 AM
kadircet marked 3 inline comments as done.Jan 13 2020, 11:35 AM
kadircet added inline comments.
clang-tools-extra/clangd/Selection.cpp
530

i would rather keep it that way to not regress performance, even if it should be negligible most of the time.

This revision was automatically updated to reflect the committed changes.