sammccall created this revision.
sammccall added a reviewer: nridge.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
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 <https://reviews.llvm.org/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
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D155381
Files:
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/index/SymbolCollector.h
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -303,10 +303,12 @@
Args, Factory->create(), Files.get(),
std::make_shared<PCHContainerOperations>());
- 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();
@@ -1956,17 +1958,25 @@
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());
}
Index: clang-tools-extra/clangd/index/SymbolCollector.h
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.h
+++ clang-tools-extra/clangd/index/SymbolCollector.h
@@ -87,6 +87,7 @@
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
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -490,7 +490,8 @@
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;
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -303,10 +303,12 @@
Args, Factory->create(), Files.get(),
std::make_shared<PCHContainerOperations>());
- 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();
@@ -1956,17 +1958,25 @@
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());
}
Index: clang-tools-extra/clangd/index/SymbolCollector.h
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.h
+++ clang-tools-extra/clangd/index/SymbolCollector.h
@@ -87,6 +87,7 @@
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
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -490,7 +490,8 @@
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;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
