For a given cursor position, it returns ranges that are interesting to the user.
Currently the semantic ranges correspond to the nodes of the syntax trees.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 37919 Build 37918: arc lint + arc unit
Event Timeline
Nice! Particularly: great tests.
Only big thing is toHalfOpenFileRange should get you substantially better macro handling.
clang-tools-extra/clangd/SemanticSelection.cpp | ||
---|---|---|
19 | nit: "unique" suggests this function checks against all the ranges in *Result (which it doesn't, and indeed doesn't need to) | |
34 | we use log() or elog() for this, make sure you include Offset.takeError() for the original message Alternatively, you might consider returning Expected<SemanticSelectionResult> so the error gets propagated to the client (as this really is a client error) | |
40 | I think you probably want to bail out once you hit TranslationUnitDecl. And maybe at namespace decls? Though I have often in my editor wondered "what's the enclosing namespace"... | |
46 | Ah, this will work if the start and the end of the AST node are written directly in the file, and no macros are involved. But try toHalfOpenFileRange in SourceCode.h, which should work in a lot more cases. In particular, this should work: #define INC(X) X + 1 int a, b; int c = INC(a * b); ^ ~ ~~~~~ ~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~ | |
clang-tools-extra/clangd/SemanticSelection.h | ||
22 | I'm not sure this struct pulls its weight, I'd suggest just returning vector<Range> here. Often we mirror the LSP here, but LSP is weird enough here (a linked list, seriously?) that I don't particularly think we should. | |
28 | incomplete sentence. It'd be nice to spell out on this line what an "interesting" range is (just one that corresponds to an AST node, right?) | |
29 | somewhere you should mention that front() is the inner most range, and back() the outermost. | |
clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp | ||
54 | this is the case where toHalfOpenFileName should help | |
60 | interesting, this *might* be worth special handling, to select the whole macro expansion. Wonder if people will complain. Let's leave it for now for simplicity | |
83 | If you're interested, I think the fix here is in pointBounds() in selection.cpp. (definitely a different patch though) | |
89 | oh, this is interesting. it's a side-effect of how selectiontree now ignores whitespace (and semicolons, and comments). We convert a cursor into a size-one selection, and that used to always select something, but no longer does. | |
116 | why commented out? |
clang-tools-extra/clangd/SemanticSelection.cpp | ||
---|---|---|
34 | Returning Expected<vector<Ranges>> now to propagate the error. | |
40 | Bailing out once we hit the TUDecl. I am not sure about exiting on namespace decl. I am keeping this for now. We can remove this later if the need be. | |
46 | Interesting. This works fine in such cases. Thanks. #define MUL ) * ( int var = (4 + 15 MUL 6 + 10); ^ ~~ ~~~~~~ ~~~~~~~~~~~ | |
clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp | ||
116 | Since the statement contains many [[ and ]], I have added the actual statement in comments to make it more readable. |
Nice!
Please make sure you run clang-format (I think it'll add missing newlines at EOF)
clang-tools-extra/clangd/SemanticSelection.cpp | ||
---|---|---|
23 | nit: it's idiomatic in LLVM to pass mutable references rather than pointers where possible | |
53 | || SM.getFileID(SR->getBegin()) != SM.getMainFileID() (begin and end are guaranteed to be the same file, but it may not be the main file) | |
clang-tools-extra/clangd/SemanticSelection.h | ||
23 | nit: if you intend to use doxygen comments (with \p syntax) you probably want /// so doxygen will render them |
selectionRange:
- https://github.com/microsoft/vscode-languageserver-node/blob/master/protocol/src/protocol.selectionRange.ts
- https://github.com/microsoft/language-server-protocol/issues/613
It hasn't been published/released yet, but AIUI it will be soon. (Thus no mention of it being an extension everywhere).
I think @usaxena95 is working on a followup to add the LSP bindings.
I'm not sure this struct pulls its weight, I'd suggest just returning vector<Range> here.
This isn't a stable API, we can change it if we need to add things.
Or is there something you have in mind?
Often we mirror the LSP here, but LSP is weird enough here (a linked list, seriously?) that I don't particularly think we should.