Skip to content

Commit d828397

Browse files
JVApenJVApen
committed
Do not restrict HeaderSourceSwitch to switching between .h and .cpp. For templates, the relevant 'source' is the header containing the implementation of the templated functions. Instead of considering every symbol in the current file as relevant, we restrict it to those that either have the declaration or definition in that file.
1 parent ad9d551 commit d828397

File tree

2 files changed

+183
-17
lines changed

2 files changed

+183
-17
lines changed

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

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -82,26 +82,36 @@ std::optional<Path> getCorrespondingHeaderOrSource(PathRef OriginalFile,
8282
Request.IDs.insert(ID);
8383
}
8484
llvm::StringMap<int> Candidates; // Target path => score.
85-
auto AwardTarget = [&](const char *TargetURI) {
86-
if (auto TargetPath = URI::resolve(TargetURI, OriginalFile)) {
87-
if (!pathEqual(*TargetPath, OriginalFile)) // exclude the original file.
88-
++Candidates[*TargetPath];
89-
} else {
90-
elog("Failed to resolve URI {0}: {1}", TargetURI, TargetPath.takeError());
91-
}
92-
};
93-
// If we switch from a header, we are looking for the implementation
94-
// file, so we use the definition loc; otherwise we look for the header file,
95-
// we use the decl loc;
85+
// When in the implementation file, we always search for the header file,
86+
// using the decl loc. When we are in a header, this usually implies searching
87+
// for implementation, for which we use the definition loc. For templates, we
88+
// can have separate implementation headers, which behave as an implementation
89+
// file. As such, we always have to add the decl loc and conditionally
90+
// definition loc.
9691
//
9792
// For each symbol in the original file, we get its target location (decl or
9893
// def) from the index, then award that target file.
99-
const bool IsHeader = isHeaderFile(OriginalFile, AST.getLangOpts());
94+
#ifdef CLANGD_PATH_CASE_INSENSITIVE
95+
auto pathEqual = [](const llvm::StringRef &l, const llvm::StringRef &r) {
96+
return l.equals_insensitive(r);
97+
};
98+
#else
99+
auto pathEqual = [](const llvm::StringRef &l, const llvm::StringRef &r) {
100+
return l.equals(r);
101+
};
102+
#endif
100103
Index->lookup(Request, [&](const Symbol &Sym) {
101-
if (IsHeader)
102-
AwardTarget(Sym.Definition.FileURI);
103-
else
104-
AwardTarget(Sym.CanonicalDeclaration.FileURI);
104+
auto TargetPathDefinition =
105+
URI::resolve(Sym.Definition.FileURI, OriginalFile);
106+
auto TargetPathDeclaration =
107+
URI::resolve(Sym.CanonicalDeclaration.FileURI, OriginalFile);
108+
if (!TargetPathDefinition || !TargetPathDeclaration)
109+
return;
110+
if (pathEqual(*TargetPathDefinition, OriginalFile)) {
111+
if (!pathEqual(*TargetPathDeclaration, OriginalFile))
112+
++Candidates[*TargetPathDeclaration];
113+
} else if (pathEqual(*TargetPathDeclaration, OriginalFile))
114+
++Candidates[*TargetPathDefinition];
105115
});
106116
// FIXME: our index doesn't have any interesting information (this could be
107117
// that the background-index is not finished), we should use the decl/def

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

Lines changed: 157 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,162 @@ TEST(HeaderSourceSwitchTest, FromHeaderToSource) {
223223
}
224224
}
225225

226+
TEST(HeaderSourceSwitchTest, FromHeaderToImplHeader) {
227+
// build a proper index, which contains symbols:
228+
// A_Sym1, declared in TestTU.h, defined in a_ext.h
229+
// B_Sym[1-2], declared in TestTU.h, defined in b_ext.h
230+
SymbolSlab::Builder AllSymbols;
231+
auto addHeader = [&](auto implName, auto declarationContent,
232+
auto implContent) {
233+
TestTU Testing;
234+
Testing.ExtraArgs.push_back(
235+
"-xc++-header"); // inform clang this is a header.
236+
237+
Testing.Filename = implName;
238+
Testing.Code = implContent;
239+
Testing.HeaderFilename = "TestTU.h";
240+
Testing.HeaderCode = declarationContent;
241+
242+
for (auto &Sym : Testing.headerSymbols())
243+
AllSymbols.insert(Sym);
244+
};
245+
246+
addHeader("a_ext.h", "template<typename T> void A_Sym1();",
247+
"template<typename T> void A_Sym1() {};");
248+
addHeader("b_ext.h", R"cpp(
249+
template<typename T> void B_Sym1();
250+
template<typename T> void B_Sym2();
251+
template<typename T> void B_Sym3_NoDef();
252+
)cpp",
253+
R"cpp(
254+
template<typename T> void B_Sym1() {}
255+
template<typename T> void B_Sym2() {}
256+
)cpp");
257+
auto Index = MemIndex::build(std::move(AllSymbols).build(), {}, {});
258+
259+
// Test for switch from .h header to .cc source
260+
struct {
261+
llvm::StringRef HeaderCode;
262+
std::optional<std::string> ExpectedSource;
263+
} TestCases[] = {
264+
{"// empty, no header found", std::nullopt},
265+
{R"cpp(
266+
// no definition found in the index.
267+
template<typename T> void NonDefinition();
268+
)cpp",
269+
std::nullopt},
270+
{R"cpp(
271+
template<typename T> void A_Sym1();
272+
)cpp",
273+
testPath("a_ext.h")},
274+
{R"cpp(
275+
// b_ext.h wins.
276+
template<typename T> void A_Sym1();
277+
template<typename T> void B_Sym1();
278+
template<typename T> void B_Sym2();
279+
)cpp",
280+
testPath("b_ext.h")},
281+
{R"cpp(
282+
// a_ext.h and b_ext.h have same scope, but a_ext.h because "a_ext.h" < "b_ext.h".
283+
template<typename T> void A_Sym1();
284+
template<typename T> void B_Sym1();
285+
)cpp",
286+
testPath("a_ext.h")},
287+
288+
{R"cpp(
289+
// We don't have definition in the index, so stay in the header.
290+
template<typename T> void B_Sym3_NoDef();
291+
)cpp",
292+
std::nullopt},
293+
};
294+
for (const auto &Case : TestCases) {
295+
TestTU TU = TestTU::withCode(Case.HeaderCode);
296+
TU.Filename = "TestTU.h";
297+
TU.ExtraArgs.push_back("-xc++-header"); // inform clang this is a header.
298+
auto HeaderAST = TU.build();
299+
EXPECT_EQ(Case.ExpectedSource,
300+
getCorrespondingHeaderOrSource(testPath(TU.Filename), HeaderAST,
301+
Index.get()));
302+
}
303+
}
304+
305+
TEST(HeaderSourceSwitchTest, FromImplHeaderToHeader) {
306+
// build a proper index, which contains symbols:
307+
// A_Sym1, declared in TestTU.h, defined in a_ext.h
308+
// B_Sym[1-2], declared in TestTU.h, defined in b_ext.h
309+
SymbolSlab::Builder AllSymbols;
310+
auto addHeader = [&](auto declName, auto declContent, auto implContent) {
311+
TestTU Testing;
312+
Testing.ExtraArgs.push_back(
313+
"-xc++-header"); // inform clang this is a header.
314+
315+
Testing.Filename = "TestTU.h";
316+
Testing.Code = implContent;
317+
Testing.HeaderFilename = declName;
318+
Testing.HeaderCode = declContent;
319+
320+
for (auto &Sym : Testing.headerSymbols())
321+
AllSymbols.insert(Sym);
322+
};
323+
324+
addHeader("a.h", "template<typename T> void A_Sym1();",
325+
"template<typename T> void A_Sym1() {};");
326+
addHeader("b.h", R"cpp(
327+
template<typename T> void B_Sym1();
328+
template<typename T> void B_Sym2();
329+
template<typename T> void B_Sym3_NoDef();
330+
)cpp",
331+
R"cpp(
332+
template<typename T> void B_Sym1() {}
333+
template<typename T> void B_Sym2() {}
334+
)cpp");
335+
auto Index = MemIndex::build(std::move(AllSymbols).build(), {}, {});
336+
337+
// Test for switch from .h header to .cc source
338+
struct {
339+
llvm::StringRef HeaderCode;
340+
std::optional<std::string> ExpectedSource;
341+
} TestCases[] = {
342+
{"// empty, no header found", std::nullopt},
343+
{R"cpp(
344+
// no definition found in the index.
345+
template<typename T> void NonDefinition(){}
346+
)cpp",
347+
std::nullopt},
348+
{R"cpp(
349+
template<typename T> void A_Sym1(){}
350+
)cpp",
351+
testPath("a.h")},
352+
{R"cpp(
353+
// b.h wins.
354+
template<typename T> void A_Sym1(){}
355+
template<typename T> void B_Sym1(){}
356+
template<typename T> void B_Sym2(){}
357+
)cpp",
358+
testPath("b.h")},
359+
{R"cpp(
360+
// a.h and b.h have same scope, but a.h because "a.h" < "b.h".
361+
template<typename T> void A_Sym1(){}
362+
template<typename T> void B_Sym1(){}
363+
)cpp",
364+
testPath("a.h")},
365+
366+
{R"cpp(
367+
// We don't have definition in the index, so stay in the header.
368+
template<typename T> void B_Sym3_NoDef();
369+
)cpp",
370+
std::nullopt},
371+
};
372+
for (const auto &Case : TestCases) {
373+
TestTU TU = TestTU::withCode(Case.HeaderCode);
374+
TU.Filename = "TestTU.h";
375+
TU.ExtraArgs.push_back("-xc++-header"); // inform clang this is a header.
376+
auto HeaderAST = TU.build();
377+
EXPECT_EQ(Case.ExpectedSource,
378+
getCorrespondingHeaderOrSource(testPath(TU.Filename), HeaderAST,
379+
Index.get()));
380+
}
381+
}
226382
TEST(HeaderSourceSwitchTest, FromSourceToHeader) {
227383
// build a proper index, which contains symbols:
228384
// A_Sym1, declared in a.h, defined in TestTU.cpp
@@ -281,7 +437,7 @@ TEST(HeaderSourceSwitchTest, FromSourceToHeader) {
281437
};
282438
for (const auto &Case : TestCases) {
283439
TestTU TU = TestTU::withCode(Case.SourceCode);
284-
TU.Filename = "Test.cpp";
440+
TU.Filename = "TestTU.cpp";
285441
auto AST = TU.build();
286442
EXPECT_EQ(Case.ExpectedResult,
287443
getCorrespondingHeaderOrSource(testPath(TU.Filename), AST,

0 commit comments

Comments
 (0)