https://github.com/HighCommander4 requested changes to this pull request.
Thanks for the PR and sorry for the long turnaround time in responding. I have two high-level pieces of feedback. --- First, I think it would be nice to cut down on the amount of new "API" we're exposing in AST.h, as well as some of the code duplication between symbol tags and highlighting modifiers. (To give an example of what code duplication I'm referring to: it's not obvious why the logic for assigning the `Declaration` tag checks for `UnresolvedUsingValueDecl`. [This comment](https://searchfox.org/llvm/rev/1e985b6ddf023af5782d48c1cce881668fdf6ceb/clang-tools-extra/clangd/SemanticHighlighting.cpp#1177-1179) explains it nicely, but rather than asking for the comment to be duplicated, I'd rather avoid the check itself being duplicated in the first place.) Here's a specific proposal: * Establish a "packed" representation for symbol tags. This can be as simple as `using SymbolTags = uint32_t;` , with the interpretation that if tag `N` is present, then the bit `1 << N` is set in the value. * (The reason for having a packed representation at all is so that we don't impose the cost of allocating vectors onto all consumers including semantic highlighting.) * Have the public API in `AST.h` just be `SymbolTags computeSymbolTags(Decl*)` or similar. * The `FindSymbols` code can call `computeSymbolTags()`, and unpack the result into a `vector<SymbolTag>`. * (If we later have multiple requests return a `vector<SymbolTag>`, it's fine to expose a function that returns a `vector<SymbolTag>` in `AST.h` as well.) * The `SemanticHighlighting` code can also call `computeSymbolTags()`, and use a mapping from `SymbolTag` to `HighlightingModifier` to convert them to highlighting modifiers. (It's fine to do this only for the subset of modifiers which are also symbol tags. The remaining modifiers can be added individually.) This way, the logic (including the `UnresolvedUsingValueDecl` check) can remain centralized in the implementation of `computeSymbolTags()`. How does that sound? --- Second, regarding testing strategy: given how relatively hard to read lit tests are, we generally use lit tests as a basic "smoke test" that the protocol conversions are working fine, while putting interesting logic into unit tests. In this case, I think it would be good to have some unit tests that exercise the new `computeSymbolTags()` API. They don't need to be extensive, I think it's sufficient to test on a small piece of code similar to the one that `symbol-tags.test` sends in its `didOpen`, that the computed tags for various symbols are the expected ones. To make this as easy as possible, I would suggest using the existing `DocumentSymbols` test fixture ([example test](https://searchfox.org/llvm/source/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp#432)), with a new matcher added around [here](https://searchfox.org/llvm/rev/1e985b6ddf023af5782d48c1cce881668fdf6ceb/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp#34-37) that checks the symbol tags. (This will exercise the `computeSymbolTags()` API indirectly via its use in the `FindSymbols` feature, but I think that's good enough.) Happy to go into more details about this if it's not clear what I'm suggesting. https://github.com/llvm/llvm-project/pull/167536 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
