Symbol tags in SymbolInformation, WorkspaceSymbol, CallHierarchyItem and TypeHierarchyItem#170103
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
6108868 to
6249e32
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
5633044 to
96218ec
Compare
96218ec to
79c265d
Compare
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
in call-hierarchy and type-hierarchy.
Fix code format.
cd4aecb to
007764f
Compare
Changed the usage of SymbolTag: - Abstract = pure virtual - Virtual = a virtual method implemented in that class, not implementing or overriding a method from base class - Implements = implements pure virtual method from base class - Override = overriding a virtual method implemented in base class - Final = final method
31a4fd5 to
2c0475f
Compare
…ierarchies_and_symbol_info
Apply consistent SymbolTag filtering for indexed C++ methods Introduce `getSymbolTags(const Symbol&)` and use it for index-backed symbol paths so tag filtering is applied consistently outside AST-based code paths. - add `getSymbolTags(const Symbol&)` in `FindSymbols` and document behavior - apply method-specific tag filtering in: - workspace symbols (`FindSymbols.cpp`) - hierarchy items (`XRefs.cpp`) - simplify `filterSymbolTags` to operate on `SymbolTags` only - keep `Declaration`/`Definition` tags for virtual/override/implements cases (only remove tags that are semantically implied, e.g. `Virtual` by `Overrides`/`Implements`/`Abstract`) - update unit tests in `FindSymbolsTests.cpp` and `SymbolCollectorTests.cpp` to assert the new expected tag sets - use `getSymbolTags(...)` in tests instead of directly expanding raw bitmasks - minor cleanup in `SymbolCollector::addDeclaration` tag assignment ordering
dd4d9fc to
317abd3
Compare
Refine filterSymbolTags semantics for method-related tags Improve SymbolTag normalization in `filterSymbolTags` by making tag precedence explicit and handling redundant combinations consistently. Changes: - Introduce local bitmask constants (`Virtual`, `Overrides`, `Implements`, `Abstract`, `Final`) for readability and maintainability. - Keep precedence logic explicit: - `Implements` removes `Overrides` and `Virtual` - `Overrides` removes `Virtual` - `Abstract` removes `Virtual` - `Final` removes redundant `Virtual`/`Overrides` - Align inline comments with actual filtering behavior. This makes tag filtering easier to reason about and avoids inconsistent symbol-tag output for C++ methods.
Replace raw integer-based SymbolTags with a dedicated strong type that is tied to SymbolTag and sanitizes invalid bits. This removes a fragile coupling between SymbolTag and SymbolTags and prevents representing non-existing tags. Update clangd tag logic and serialization paths accordingly: - switch tag construction/checks to SymbolTags::fromTag()/typed operations - keep existing on-disk bit positions compatible - adapt RIFF/YAML serialization to read/write raw SymbolTags storage safely - update serialization tests to use the typed SymbolTags API This improves robustness and maintainability while preserving behavior and backward compatibility of serialized tag values.
|
@ratzdi you requested a review but you did not undo the changes we talked about? |
This reverts commit 765ef3e
timon-ul
left a comment
There was a problem hiding this comment.
Looks good to me now, @HighCommander4 please have a look next :)
|
@ArcsinX if you happen to find the time before @HighCommander4 does, it would be appreciated if you could review this. |
I plan to take a look this week. P.S. Sorry for the late reply, I’ve been really busy over the past couple of weeks. |
| @@ -160,6 +161,10 @@ TEST(SerializationTest, YAMLConversions) { | |||
| EXPECT_EQ(static_cast<uint8_t>(Sym1.Flags), 129); | |||
| EXPECT_TRUE(Sym1.Flags & Symbol::IndexedForCodeCompletion); | |||
| EXPECT_FALSE(Sym1.Flags & Symbol::Deprecated); | |||
| // Tags: Deprecated (1<<1=2) | Static (1<<8=256) = 258 | |||
| EXPECT_EQ(Sym1.Tags, 258u); | |||
| EXPECT_TRUE(Sym1.Tags & (1u << static_cast<unsigned>(SymbolTag::Deprecated))); | |||
There was a problem hiding this comment.
static_cast<unsigned>(SymbolTag::...) appears quite frequently in this PR, but we have toSymbolTagBitmask() function for this. Maybe it's better to use toSymbolTagBitmask()?
| // virtual) | ||
| bool isOverrides(const NamedDecl *ND) { | ||
| if (const auto *MD = llvm::dyn_cast<CXXMethodDecl>(ND)) { | ||
| if (MD->size_overridden_methods() == 0) |
There was a problem hiding this comment.
I think we don't need to check MD->size_overridden_methods() here, because at line 150 we have a loop which will be just no-op, if there are not overridden methods.
|
|
||
| for (const auto *Overridden : MD->overridden_methods()) { | ||
| // Check if the overridden method is virtual but not pure virtual | ||
| if (Overridden->isVirtual() && !Overridden->isPureVirtual()) |
There was a problem hiding this comment.
Instead of checking Overridden->isVirtual() at every loop iteration, we can check MD->isVirtual() before the loop and return false is not.
I mean that Overridden is virtual iff MD is virtual. Maybe inside this loop we can add an assertion for Overridden->isVirtual().
I.e.
- check
MD->isVirtual()before the loop - add assertion for
Overridden->isVirtual()inside the loop
| // 3. ALL overridden methods are pure virtual | ||
| bool isImplements(const NamedDecl *ND) { | ||
| if (const auto *MD = llvm::dyn_cast<CXXMethodDecl>(ND)) { | ||
| if (MD->size_overridden_methods() == 0 || MD->isPureVirtual()) |
There was a problem hiding this comment.
Same here. I think we don't need to check MD->size_overridden_methods()
| const SymbolTags AbstractMask = toSymbolTagBitmask(SymbolTag::Abstract); | ||
| const SymbolTags FinalMask = toSymbolTagBitmask(SymbolTag::Final); | ||
|
|
||
| const SymbolTags RemoveVirtualAndOverrides = VirtualMask | OverridesMask; |
There was a problem hiding this comment.
I think the name of this variable should be VirtualAndOverridesMask, reflecting what it holds.
This PR is continuation of #167536.
What is new?
It adds code to compute symbol tags for the following types:
SymbolInformation
WorkspaceSymbol
CallHierarchyItem
TypeHierarchyItem
Calculation of the symbol tags
OverridesandImplementsCalculation of symbol tags during the AST creation and embedding them in
Symbol.Minor change on
struct Symbolto minimize paddings and thus reducing momory consumption.Related issues: