This is an archive of the discontinued LLVM Phabricator instance.

Use pseudo parser for folding ranges
ClosedPublic

Authored by usaxena95 on Jul 13 2022, 8:08 AM.

Details

Summary

This first version only uses bracket matching.
We plan to extend this to use DirectiveTree as well.

Also includes changes to Token to allow retrieving corresponding token
in token stream of original source file.

Diff Detail

Event Timeline

usaxena95 created this revision.Jul 13 2022, 8:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 8:08 AM
usaxena95 requested review of this revision.Jul 13 2022, 8:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 8:08 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Addressed offline comments.

usaxena95 edited the summary of this revision. (Show Details)Jul 14 2022, 12:09 PM
usaxena95 added reviewers: sammccall, hokein.

Removed changes from previous revision.

hokein added inline comments.Jul 15 2022, 3:20 AM
clang-tools-extra/clangd/SemanticSelection.cpp
183

nit: no need to use llvm::Optional.

186

Do we want to strip comments? I think we can keep the comments.

193

The if logic seems tricky, it is doing two different things:

  1. avoid creating duplicate FoldingRange for a pair bracket
  2. avoid creating a FoldingRange if the pair bracket is on the same line.

Are both intended?

195

the OrigStream.origToken syntax seems weird from the caller side (it is counter-intuitive).

I think OrigStream.tokens()[T.OrigTokIdx] is clearer.

clang-tools-extra/pseudo/include/clang-pseudo/Token.h
71

I'd suggest initializing it to Index::Invalid.

it'd be nice to add some tests in the pseudo/unittests/TokenTest.cpp.

181

I'm not sure we should have this API, it seems error-prone to use, and it only makes sense for the original token stream. (I'd just remove it, and use OrigStream.tokens()[T.OrigTokIdx] directly, see my other comment)

sammccall added inline comments.Jul 15 2022, 3:21 AM
clang-tools-extra/clangd/CMakeLists.txt
149

It should rather be up to the clangPseudo target to specify its include directories, if it doesn't already

I think interface_include_directories() is the right function (though knowing llvm maybe it's wrapped somehow)

https://cmake.org/cmake/help/latest/prop_tgt/INTERFACE_INCLUDE_DIRECTORIES.html

clang-tools-extra/clangd/SemanticSelection.cpp
164

we should transfer this fixme, either now or when we remove this code

184

This means we won't offer folding ranges in disabled-PP regions.
This is a significant regression of the client-side fallback!
It's not trivial to address though. Add a FIXME?

186

Stripping comments doesn't seem necessary to me, maybe I'm missing something.

clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
263

You've removed the tests for the old implementation, but it's still used by ClangdServer.

Test both versions instead?

clang-tools-extra/pseudo/include/clang-pseudo/Token.h
70

nit: not clear what "original source file" is opposed to

maybe (as raw-lexed from the source code)

71

OriginalIndex (avoid cryptic abbreviations, "token" is implied)

175

I don't like the stream knowing about this attribute, there's no reasonable way to actually use it dynamically at runtime, we're complicating the API just so we can add some assertions.

What's wrong with OrigToken = OrigStream.tokens()[T.OrigIndex]?

(If we're going to teach the stream about the "derived stream" concept, it seems more natural to give it a pointer to its parent and allow mapping to any ancestor stream)

clang-tools-extra/pseudo/lib/Lex.cpp
30

nit: unsigned -> Token::Index
Idx -> Index? (it's not too cryptic here but also only saving 2 chars)

usaxena95 updated this revision to Diff 444975.Jul 15 2022, 7:13 AM
usaxena95 marked 15 inline comments as done.

Addressed comments.

clang-tools-extra/clangd/SemanticSelection.cpp
193

Yes. Both of these are intentional. Added a comment.

usaxena95 updated this revision to Diff 445002.Jul 15 2022, 8:50 AM

Removed unused headers and fix spellings.

hokein accepted this revision.Jul 18 2022, 1:44 AM

Thanks, this looks good.

This revision is now accepted and ready to land.Jul 18 2022, 1:44 AM
This revision was automatically updated to reflect the committed changes.