[clang-tools-extra] 290a98c - [clangd] Allow indexing of __reserved_names outside system headers

[Sam McCall via cfe-commits]

Author: Sam McCall
Date: 2023-07-21T23:30:31+02:00
New Revision: 290a98c7b00899b6aba0fc892e8f29fecc00a82e

URL: https://github.com/llvm/llvm-project/commit/290a98c7b00899b6aba0fc892e8f29fecc00a82e
DIFF: https://github.com/llvm/llvm-project/commit/290a98c7b00899b6aba0fc892e8f29fecc00a82e.diff

LOG: [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

Added: 
    

Modified: 
    clang-tools-extra/clangd/index/SymbolCollector.cpp
    clang-tools-extra/clangd/index/SymbolCollector.h
    clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index 214991a0674cc6..e9522171d70b75 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 0a890c4d5760a6..fe0de1925ecf2f 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.h
+++ b/clang-tools-extra/clangd/index/SymbolCollector.h
@@ -88,6 +88,7 @@ class SymbolCollector : public index::IndexDataConsumer {
     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 32a47c5ebfe865..774dcb8c60d1a1 100644
--- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -304,10 +304,12 @@ class SymbolCollectorTest : public ::testing::Test {
         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 
diff erent 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());
 }