Skip to content

Commit 317abd3

Browse files
ratzdiratzdi
committed
Review: code review adjustments.
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
1 parent f99012b commit 317abd3

6 files changed

Lines changed: 93 additions & 70 deletions

File tree

‎clang-tools-extra/clangd/FindSymbols.cpp‎

Lines changed: 36 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -120,16 +120,6 @@ bool isAbstract(const Decl *D) {
120120
}
121121

122122
// Indicates whether declaration D is virtual in cases where D is a method.
123-
// We want to treat a method as virtual if it is declared virtual, even if it
124-
// is not implemented in this class, or if it overrides/implements a
125-
// base-class method. This is because the "virtual" modifier is still relevant
126-
// to the method's behavior and how it should be highlighted, even if it is
127-
// not itself a virtual method in the strictest sense. For example, a method
128-
// that overrides a virtual method from a base class is still considered
129-
// virtual, even if it is not declared as such in the derived class.
130-
// Similarly, a method that implements a pure virtual method from a base class
131-
// is also considered virtual, even if it is not declared as such in the
132-
// derived class.
133123
bool isVirtual(const Decl *D) {
134124
if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(D))
135125
return CMD->isVirtual();
@@ -206,20 +196,15 @@ bool isUniqueDefinition(const NamedDecl *Decl) {
206196
// Filter symbol tags based on the presence of other tags and the kind of
207197
// symbol. This is needed to avoid redundant tags, e.g. final implies override,
208198
// override implies virtual, etc.
209-
SymbolTags filterSymbolTags(const NamedDecl &ND, const SymbolTags ST) {
199+
SymbolTags filterSymbolTags(const SymbolTags ST) {
210200
SymbolTags Result = ST;
211201

212-
if (not isa<CXXMethodDecl>(ND))
213-
return Result;
214-
215202
if (ST & toSymbolTagBitmask(SymbolTag::Overrides)) {
216203
// Overrides means that ND overrides an existing implementation of a virtual
217204
// method in a base class. If a symbol is marked as Overrides, the tags
218205
// Virtual, Declaration and Definition should be removed, as the Overrides
219206
// tag implies that the symbol has/is virtual/declaration/definition.
220207
Result &= ~toSymbolTagBitmask(SymbolTag::Virtual);
221-
Result &= ~toSymbolTagBitmask(SymbolTag::Declaration);
222-
Result &= ~toSymbolTagBitmask(SymbolTag::Definition);
223208
}
224209
if (ST & toSymbolTagBitmask(SymbolTag::Implements)) {
225210
// Implements means that ND implements an existing pure virtual method in a
@@ -228,26 +213,14 @@ SymbolTags filterSymbolTags(const NamedDecl &ND, const SymbolTags ST) {
228213
// Implements tag implies that the symbol is virtual, is a declaration, is a
229214
// definition, and overrides a method.
230215
Result &= ~toSymbolTagBitmask(SymbolTag::Virtual);
231-
Result &= ~toSymbolTagBitmask(SymbolTag::Declaration);
232-
Result &= ~toSymbolTagBitmask(SymbolTag::Definition);
233216
Result &= ~toSymbolTagBitmask(SymbolTag::Overrides);
234217
}
235-
if (ST & toSymbolTagBitmask(SymbolTag::Virtual)) {
236-
// Virtual means that ND is a virtual method that does not override any
237-
// method in a base class. If a symbol is marked as Virtual, the tags
238-
// Declaration and Definition should be removed, as the Virtual tag implies
239-
// that the symbol is a declaration/definition.
240-
Result &= ~toSymbolTagBitmask(SymbolTag::Declaration);
241-
Result &= ~toSymbolTagBitmask(SymbolTag::Definition);
242-
}
243218
if (ST & toSymbolTagBitmask(SymbolTag::Abstract)) {
244219
// Abstract means that ND is a pure virtual method. If a symbol is marked as
245220
// Abstract, the tags Virtual, Declaration and Definition should be removed,
246221
// as the Abstract tag implies that the symbol is virtual and a
247222
// declaration/definition.
248223
Result &= ~toSymbolTagBitmask(SymbolTag::Virtual);
249-
Result &= ~toSymbolTagBitmask(SymbolTag::Declaration);
250-
Result &= ~toSymbolTagBitmask(SymbolTag::Definition);
251224
}
252225
if (ST & toSymbolTagBitmask(SymbolTag::Final)) {
253226
// Final means that ND is a method that cannot be overridden by any method
@@ -259,6 +232,24 @@ SymbolTags filterSymbolTags(const NamedDecl &ND, const SymbolTags ST) {
259232
}
260233
return Result;
261234
}
235+
236+
bool isCXXClassMethod(const clang::clangd::Symbol &S) {
237+
using clang::index::SymbolKind;
238+
using clang::index::SymbolLanguage;
239+
240+
if (S.SymInfo.Lang != SymbolLanguage::CXX)
241+
return false;
242+
243+
return llvm::is_contained({SymbolKind::InstanceMethod,
244+
SymbolKind::StaticMethod, SymbolKind::Constructor,
245+
SymbolKind::Destructor,
246+
SymbolKind::ConversionFunction},
247+
S.SymInfo.Kind);
248+
}
249+
250+
template <typename E> constexpr E enumIncrement(E Value) {
251+
return static_cast<E>(static_cast<std::underlying_type_t<E>>(Value) + 1);
252+
}
262253
} // namespace
263254

264255
SymbolTags toSymbolTagBitmask(const SymbolTag ST) {
@@ -343,25 +334,31 @@ std::vector<SymbolTag> expandTagBitmask(const SymbolTags STGS) {
343334
return Tags;
344335
}
345336

337+
std::vector<SymbolTag> getSymbolTags(const Symbol &S) {
338+
const SymbolTags Tags =
339+
isCXXClassMethod(S) ? filterSymbolTags(S.Tags) : S.Tags;
340+
return expandTagBitmask(Tags);
341+
}
342+
346343
std::vector<SymbolTag> getSymbolTags(const NamedDecl &ND) {
347-
const auto symbolTags = computeSymbolTags(ND);
344+
const auto STGS = computeSymbolTags(ND);
345+
SymbolTags FilteredTags = STGS;
348346
std::vector<SymbolTag> Tags;
349347

350-
if (symbolTags == 0)
348+
if (STGS == 0)
351349
return Tags;
352350

353-
// Apply specific filter to the symbol tags.
354-
const auto filteredTags = filterSymbolTags(ND, symbolTags);
351+
// Apply specific filter to the symbol tags only on CXX class methods.
352+
if (isa<CXXMethodDecl>(ND))
353+
FilteredTags = filterSymbolTags(STGS);
355354

356355
// Iterate through SymbolTag enum values and collect any that are present in
357356
// the bitmask. SymbolTag values are in the numeric range
358357
// [FirstTag .. LastTag].
359-
constexpr unsigned MinTag = static_cast<unsigned>(SymbolTag::FirstTag);
360-
constexpr unsigned MaxTag = static_cast<unsigned>(SymbolTag::LastTag);
361-
for (unsigned I = MinTag; I <= MaxTag; ++I) {
362-
auto ST = static_cast<SymbolTag>(I);
363-
if (filteredTags & toSymbolTagBitmask(ST))
364-
Tags.push_back(ST);
358+
for (SymbolTag Tag = SymbolTag::FirstTag; Tag <= SymbolTag::LastTag;
359+
Tag = enumIncrement(Tag)) {
360+
if (FilteredTags & toSymbolTagBitmask(Tag))
361+
Tags.push_back(Tag);
365362
}
366363
return Tags;
367364
}
@@ -497,7 +494,7 @@ getWorkspaceSymbols(llvm::StringRef Query, int Limit,
497494
Info.score = Relevance.NameMatch > std::numeric_limits<float>::epsilon()
498495
? Score / Relevance.NameMatch
499496
: QualScore;
500-
Info.tags = expandTagBitmask(Sym.Tags);
497+
Info.tags = getSymbolTags(Sym);
501498
Top.push({Score, std::move(Info)});
502499
});
503500
for (auto &R : std::move(Top).items())

‎clang-tools-extra/clangd/FindSymbols.h‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,19 @@ SymbolTags computeSymbolTags(const NamedDecl &ND);
6363
/// \p ND The declaration to get tags for.
6464
std::vector<SymbolTag> getSymbolTags(const NamedDecl &ND);
6565

66-
/// Expands a SymbolTags bitmask into individual SymbolTag values.
67-
std::vector<SymbolTag> expandTagBitmask(SymbolTags STGS);
68-
66+
/// Returns the \c SymbolTag values for the given indexed \p S.
67+
///
68+
/// Converts the bitmask stored in \c Symbol::Tags into a flat vector of
69+
/// \c SymbolTag enum values. For C++ class methods (instance methods, static
70+
/// methods, constructors, destructors, and conversion functions), a semantic
71+
/// filter is applied first to remove tags that are implied by higher-priority
72+
/// tags (e.g. \c Overrides implies \c Virtual, so \c Virtual is suppressed).
73+
/// For all other symbol kinds the bitmask is expanded as-is.
74+
///
75+
/// \param S The indexed symbol whose tags should be returned.
76+
/// \return A vector of \c SymbolTag values present in \c S.Tags, after
77+
/// applying any applicable filters.
78+
std::vector<SymbolTag> getSymbolTags(const Symbol &S);
6979
} // namespace clangd
7080
} // namespace clang
7181

‎clang-tools-extra/clangd/XRefs.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1873,7 +1873,7 @@ static std::optional<HierarchyItem> symbolToHierarchyItem(const Symbol &S,
18731873
HI.name = std::string(S.Name);
18741874
HI.detail = (S.Scope + S.Name).str();
18751875
HI.kind = indexSymbolKindToSymbolKind(S.SymInfo);
1876-
HI.tags = expandTagBitmask(S.Tags);
1876+
HI.tags = getSymbolTags(S);
18771877
HI.selectionRange = Loc->range;
18781878
// FIXME: Populate 'range' correctly
18791879
// (https://github.com/clangd/clangd/issues/59).

‎clang-tools-extra/clangd/index/SymbolCollector.cpp‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1127,9 +1127,6 @@ const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
11271127
S.Documentation = Documentation;
11281128
}
11291129
};
1130-
1131-
S.Tags = computeSymbolTags(ND);
1132-
11331130
if (!(S.Flags & Symbol::IndexedForCodeCompletion)) {
11341131
if (Opts.StoreAllDocumentation)
11351132
UpdateDoc();
@@ -1142,6 +1139,7 @@ const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
11421139
getSignature(*CCS, &Signature, &SnippetSuffix, SymbolCompletion.Kind,
11431140
SymbolCompletion.CursorKind);
11441141
S.Signature = Signature;
1142+
S.Tags = computeSymbolTags(ND);
11451143
S.CompletionSnippetSuffix = SnippetSuffix;
11461144
std::string ReturnType = getReturnType(*CCS);
11471145
S.ReturnType = ReturnType;

‎clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp‎

Lines changed: 36 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1181,10 +1181,12 @@ TEST(DocumentSymbolsTest, SymbolTagsMustContainPublicAbstract) {
11811181
AllOf(withName("A"),
11821182
children(AllOf(withName("f1"),
11831183
withSymbolTags(SymbolTag::Public,
1184+
SymbolTag::Declaration,
11841185
SymbolTag::Abstract)))),
11851186
AllOf(withName("B"),
11861187
children(AllOf(withName("f2"),
11871188
withSymbolTags(SymbolTag::Public,
1189+
SymbolTag::Declaration,
11881190
SymbolTag::Abstract))))));
11891191
}
11901192

@@ -1212,17 +1214,23 @@ TEST(DocumentSymbolsTest, SymbolTagsMustContainPublicVirtualAndOverrides) {
12121214
EXPECT_THAT(
12131215
Symbols,
12141216
UnorderedElementsAre(
1215-
AllOf(withName("A"),
1216-
children(
1217-
AllOf(withName("f1"), withSymbolTags(SymbolTag::Public,
1218-
SymbolTag::Virtual)))),
1217+
AllOf(
1218+
withName("A"),
1219+
children(AllOf(
1220+
withName("f1"),
1221+
withSymbolTags(SymbolTag::Public, SymbolTag::Declaration,
1222+
SymbolTag::Definition, SymbolTag::Virtual)))),
12191223
AllOf(withName("B"),
1220-
children(AllOf(
1221-
withName("f1"),
1222-
withSymbolTags(SymbolTag::Public, SymbolTag::Overrides)))),
1224+
children(AllOf(withName("f1"),
1225+
withSymbolTags(SymbolTag::Public,
1226+
SymbolTag::Declaration,
1227+
SymbolTag::Definition,
1228+
SymbolTag::Overrides)))),
12231229
AllOf(withName("C"),
12241230
children(AllOf(withName("f1"),
12251231
withSymbolTags(SymbolTag::Public,
1232+
SymbolTag::Declaration,
1233+
SymbolTag::Definition,
12261234
SymbolTag::Overrides))))));
12271235
}
12281236

@@ -1258,18 +1266,25 @@ TEST(DocumentSymbolsTest,
12581266
AllOf(withName("A"),
12591267
children(AllOf(withName("f1"),
12601268
withSymbolTags(SymbolTag::Public,
1269+
SymbolTag::Declaration,
12611270
SymbolTag::Abstract)))),
12621271
AllOf(withName("B"),
12631272
children(AllOf(withName("f1"),
12641273
withSymbolTags(SymbolTag::Public,
1274+
SymbolTag::Declaration,
1275+
SymbolTag::Definition,
12651276
SymbolTag::Implements)))),
12661277
AllOf(withName("C"),
12671278
children(AllOf(withName("f1"),
12681279
withSymbolTags(SymbolTag::Public,
1280+
SymbolTag::Declaration,
1281+
SymbolTag::Definition,
12691282
SymbolTag::Overrides)))),
12701283
AllOf(withName("D"),
12711284
children(AllOf(withName("f1"),
12721285
withSymbolTags(SymbolTag::Public,
1286+
SymbolTag::Declaration,
1287+
SymbolTag::Definition,
12731288
SymbolTag::Final))))));
12741289
}
12751290

@@ -1306,9 +1321,12 @@ TEST(DocumentSymbolsTest, SymbolTagsCompilation) {
13061321
SymbolTag::Definition),
13071322
children(
13081323
AllOf(withName("~A"),
1309-
withSymbolTags(SymbolTag::Public, SymbolTag::Virtual)),
1310-
AllOf(withName("f1"),
1311-
withSymbolTags(SymbolTag::Public, SymbolTag::Abstract)),
1324+
withSymbolTags(SymbolTag::Public, SymbolTag::Virtual,
1325+
SymbolTag::Declaration,
1326+
SymbolTag::Definition)),
1327+
AllOf(withName("f1"), withSymbolTags(SymbolTag::Public,
1328+
SymbolTag::Declaration,
1329+
SymbolTag::Abstract)),
13121330
AllOf(withName("f2"), withSymbolTags(SymbolTag::Public,
13131331
SymbolTag::Declaration,
13141332
SymbolTag::ReadOnly)),
@@ -1322,13 +1340,14 @@ TEST(DocumentSymbolsTest, SymbolTagsCompilation) {
13221340
AllOf(withName("A::f2"),
13231341
withSymbolTags(SymbolTag::Public, SymbolTag::Declaration,
13241342
SymbolTag::Definition, SymbolTag::ReadOnly)),
1325-
AllOf(
1326-
withName("B"),
1327-
withSymbolTags(SymbolTag::Final, SymbolTag::Declaration,
1328-
SymbolTag::Definition),
1329-
children(AllOf(withName("f1"),
1330-
withSymbolTags(SymbolTag::Public, SymbolTag::Final,
1331-
SymbolTag::Implements))))));
1343+
AllOf(withName("B"),
1344+
withSymbolTags(SymbolTag::Final, SymbolTag::Declaration,
1345+
SymbolTag::Definition),
1346+
children(AllOf(
1347+
withName("f1"),
1348+
withSymbolTags(SymbolTag::Public, SymbolTag::Declaration,
1349+
SymbolTag::Definition, SymbolTag::Final,
1350+
SymbolTag::Implements))))));
13321351
}
13331352

13341353
} // namespace

‎clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1362,20 +1362,19 @@ TEST_F(SymbolCollectorTest, SymbolTagsWithIndexing) {
13621362

13631363
runSymbolCollector(Header, /*Main=*/"");
13641364
const Symbol &A = findSymbol(Symbols, "A");
1365-
EXPECT_THAT(expandTagBitmask(A.Tags),
1365+
EXPECT_THAT(getSymbolTags(A),
13661366
UnorderedElementsAre(SymbolTag::Abstract, SymbolTag::Declaration,
13671367
SymbolTag::Definition));
13681368

13691369
const Symbol &B = findSymbol(Symbols, "B");
1370-
EXPECT_THAT(expandTagBitmask(B.Tags),
1370+
EXPECT_THAT(getSymbolTags(B),
13711371
UnorderedElementsAre(SymbolTag::Final, SymbolTag::Declaration,
13721372
SymbolTag::Definition));
13731373
const Symbol &Bf1 = findSymbol(Symbols, "B::f1");
1374-
EXPECT_THAT(expandTagBitmask(Bf1.Tags),
1375-
UnorderedElementsAre(SymbolTag::Public, SymbolTag::Final,
1376-
SymbolTag::Virtual, SymbolTag::Declaration,
1377-
SymbolTag::Definition,
1378-
SymbolTag::Implements));
1374+
EXPECT_THAT(getSymbolTags(Bf1),
1375+
UnorderedElementsAre(
1376+
SymbolTag::Public, SymbolTag::Final, SymbolTag::Declaration,
1377+
SymbolTag::Definition, SymbolTag::Implements));
13791378
}
13801379

13811380
TEST_F(SymbolCollectorTest, ObjCOverrideRelationsSimpleInheritance) {

0 commit comments

Comments
 (0)