From 290a98c7b00899b6aba0fc892e8f29fecc00a82e Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Sat, 15 Jul 2023 22:21:25 +0200 Subject: [PATCH] [clangd] Allow indexing of __reserved_names outside system headers The special handling for these names was added in https://github.com/llvm/llvm-project/commit/055d8090d1d5137dab88533995e0c5d9b5390c28 and the motivation was the C++ standard library. It turns out some projects like using these names anyway, in particular the linux kernel. D153946 proposed making this a config option, but there are some implementation issues with the config system. As an alternative, this patch tweaks the heuristic so we only drop these symbols in system headers. This does the right thing for linux and the C++ standard library, at least. Fixes https://github.com/clangd/clangd/issues/1680 Fixes https://github.com/llvm/llvm-project/issues/63862 Differential Revision: https://reviews.llvm.org/D155381 --- clang-tools-extra/clangd/index/SymbolCollector.cpp | 3 ++- clang-tools-extra/clangd/index/SymbolCollector.h | 1 + .../clangd/unittests/SymbolCollectorTests.cpp | 24 ++++++++++++++++------ 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp index 214991a..e952217 100644 --- a/clang-tools-extra/clangd/index/SymbolCollector.cpp +++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp @@ -516,7 +516,8 @@ bool SymbolCollector::shouldCollectSymbol(const NamedDecl &ND, if (isPrivateProtoDecl(ND)) return false; if (!Opts.CollectReserved && - (hasReservedName(ND) || hasReservedScope(*ND.getDeclContext()))) + (hasReservedName(ND) || hasReservedScope(*ND.getDeclContext())) && + ASTCtx.getSourceManager().isInSystemHeader(ND.getLocation())) return false; return true; diff --git a/clang-tools-extra/clangd/index/SymbolCollector.h b/clang-tools-extra/clangd/index/SymbolCollector.h index 0a890c4..fe0de19 100644 --- a/clang-tools-extra/clangd/index/SymbolCollector.h +++ b/clang-tools-extra/clangd/index/SymbolCollector.h @@ -88,6 +88,7 @@ public: bool CollectMainFileRefs = false; /// Collect symbols with reserved names, like __Vector_base. /// This does not currently affect macros (many like _WIN32 are important!) + /// This only affects system headers. bool CollectReserved = false; /// If set to true, SymbolCollector will collect doc for all symbols. /// Note that documents of symbols being indexed for completion will always diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp index 32a47c5..774dcb8 100644 --- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp +++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp @@ -304,10 +304,12 @@ public: Args, Factory->create(), Files.get(), std::make_shared()); - InMemoryFileSystem->addFile(TestHeaderName, 0, - llvm::MemoryBuffer::getMemBuffer(HeaderCode)); - InMemoryFileSystem->addFile(TestFileName, 0, - llvm::MemoryBuffer::getMemBuffer(MainCode)); + // Multiple calls to runSymbolCollector with different contents will fail + // to update the filesystem! Why are we sharing one across tests, anyway? + EXPECT_TRUE(InMemoryFileSystem->addFile( + TestHeaderName, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode))); + EXPECT_TRUE(InMemoryFileSystem->addFile( + TestFileName, 0, llvm::MemoryBuffer::getMemBuffer(MainCode))); Invocation.run(); Symbols = Factory->Collector->takeSymbols(); Refs = Factory->Collector->takeRefs(); @@ -1788,6 +1790,8 @@ TEST_F(SymbolCollectorTest, Origin) { runSymbolCollector("class Foo {};", /*Main=*/""); EXPECT_THAT(Symbols, UnorderedElementsAre( Field(&Symbol::Origin, SymbolOrigin::Static))); + InMemoryFileSystem = new llvm::vfs::InMemoryFileSystem; + CollectorOpts.CollectMacro = true; runSymbolCollector("#define FOO", /*Main=*/""); EXPECT_THAT(Symbols, UnorderedElementsAre( Field(&Symbol::Origin, SymbolOrigin::Static))); @@ -1941,17 +1945,25 @@ TEST_F(SymbolCollectorTest, NoCrashOnObjCMethodCStyleParam) { TEST_F(SymbolCollectorTest, Reserved) { const char *Header = R"cpp( + #pragma once void __foo(); namespace _X { int secret; } )cpp"; CollectorOpts.CollectReserved = true; - runSymbolCollector("", Header); + runSymbolCollector(Header, ""); EXPECT_THAT(Symbols, UnorderedElementsAre(qName("__foo"), qName("_X"), qName("_X::secret"))); CollectorOpts.CollectReserved = false; - runSymbolCollector("", Header); // + runSymbolCollector(Header, ""); + EXPECT_THAT(Symbols, UnorderedElementsAre(qName("__foo"), qName("_X"), + qName("_X::secret"))); + + // Ugly: for some reason we reuse the test filesystem across tests. + // You can't overwrite the same filename with new content! + InMemoryFileSystem = new llvm::vfs::InMemoryFileSystem; + runSymbolCollector("#pragma GCC system_header\n" + std::string(Header), ""); EXPECT_THAT(Symbols, IsEmpty()); } -- 2.7.4