This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rG055d8090d1d5: [clangd] Don't index __reserved_names in
headers. (authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D115301?vs=392579&id=397582#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115301/new/
https://reviews.llvm.org/D115301
Files:
clang-tools-extra/clangd/AST.cpp
clang-tools-extra/clangd/AST.h
clang-tools-extra/clangd/Quality.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/index/FileIndex.cpp
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/index/SymbolCollector.h
clang-tools-extra/clangd/unittests/ASTTests.cpp
clang-tools-extra/clangd/unittests/QualityTests.cpp
clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
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
@@ -17,7 +17,6 @@
#include "clang/Index/IndexingOptions.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
-#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/VirtualFileSystem.h"
@@ -1849,6 +1848,22 @@
UnorderedElementsAre(QName("Foo"), QName("Foo::fun:")));
}
+TEST_F(SymbolCollectorTest, Reserved) {
+ const char *Header = R"cpp(
+ void __foo();
+ namespace _X { int secret; }
+ )cpp";
+
+ CollectorOpts.CollectReserved = true;
+ runSymbolCollector("", Header);
+ EXPECT_THAT(Symbols, UnorderedElementsAre(QName("__foo"), QName("_X"),
+ QName("_X::secret")));
+
+ CollectorOpts.CollectReserved = false;
+ runSymbolCollector("", Header); //
+ EXPECT_THAT(Symbols, IsEmpty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -315,6 +315,16 @@
}
}
+TEST(SourceCodeTests, isReservedName) {
+ EXPECT_FALSE(isReservedName(""));
+ EXPECT_FALSE(isReservedName("_"));
+ EXPECT_FALSE(isReservedName("foo"));
+ EXPECT_FALSE(isReservedName("_foo"));
+ EXPECT_TRUE(isReservedName("__foo"));
+ EXPECT_TRUE(isReservedName("_Foo"));
+ EXPECT_FALSE(isReservedName("foo__bar")) << "FIXME";
+}
+
TEST(SourceCodeTests, CollectIdentifiers) {
auto Style = format::getLLVMStyle();
auto IDs = collectIdentifiers(R"cpp(
Index: clang-tools-extra/clangd/unittests/QualityTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/QualityTests.cpp
+++ clang-tools-extra/clangd/unittests/QualityTests.cpp
@@ -54,13 +54,6 @@
auto AST = Header.build();
SymbolQualitySignals Quality;
- Quality.merge(findSymbol(Symbols, "_X"));
- EXPECT_FALSE(Quality.Deprecated);
- EXPECT_FALSE(Quality.ImplementationDetail);
- EXPECT_TRUE(Quality.ReservedName);
- EXPECT_EQ(Quality.References, SymbolQualitySignals().References);
- EXPECT_EQ(Quality.Category, SymbolQualitySignals::Variable);
-
Quality.merge(findSymbol(Symbols, "X_Y_Decl"));
EXPECT_TRUE(Quality.ImplementationDetail);
@@ -83,6 +76,16 @@
Quality = {};
Quality.merge(CodeCompletionResult("if"));
EXPECT_EQ(Quality.Category, SymbolQualitySignals::Keyword);
+
+ // Testing ReservedName in main file, we don't index those symbols in headers.
+ auto MainAST = TestTU::withCode("int _X;").build();
+ SymbolSlab MainSymbols = std::get<0>(indexMainDecls(MainAST));
+
+ Quality = {};
+ Quality.merge(findSymbol(MainSymbols, "_X"));
+ EXPECT_FALSE(Quality.Deprecated);
+ EXPECT_FALSE(Quality.ImplementationDetail);
+ EXPECT_TRUE(Quality.ReservedName);
}
TEST(QualityTests, SymbolRelevanceSignalExtraction) {
Index: clang-tools-extra/clangd/unittests/ASTTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ASTTests.cpp
@@ -427,6 +427,29 @@
Contains(attrKind(attr::Unlikely)));
}
+TEST(ClangdAST, HasReservedName) {
+ ParsedAST AST = TestTU::withCode(R"cpp(
+ void __foo();
+ namespace std {
+ inline namespace __1 { class error_code; }
+ namespace __detail { int secret; }
+ }
+ )cpp")
+ .build();
+
+ EXPECT_TRUE(hasReservedName(findUnqualifiedDecl(AST, "__foo")));
+ EXPECT_FALSE(
+ hasReservedScope(*findUnqualifiedDecl(AST, "__foo").getDeclContext()));
+
+ EXPECT_FALSE(hasReservedName(findUnqualifiedDecl(AST, "error_code")));
+ EXPECT_FALSE(hasReservedScope(
+ *findUnqualifiedDecl(AST, "error_code").getDeclContext()));
+
+ EXPECT_FALSE(hasReservedName(findUnqualifiedDecl(AST, "secret")));
+ EXPECT_TRUE(
+ hasReservedScope(*findUnqualifiedDecl(AST, "secret").getDeclContext()));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/index/SymbolCollector.h
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.h
+++ clang-tools-extra/clangd/index/SymbolCollector.h
@@ -81,6 +81,9 @@
bool CollectMainFileSymbols = true;
/// Collect references to main-file symbols.
bool CollectMainFileRefs = false;
+ /// Collect symbols with reserved names, like __Vector_base.
+ /// This does not currently affect macros (many like _WIN32 are important!)
+ 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
/// be collected regardless of this option.
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -356,6 +356,10 @@
// Avoid indexing internal symbols in protobuf generated headers.
if (isPrivateProtoDecl(ND))
return false;
+ if (!Opts.CollectReserved &&
+ (hasReservedName(ND) || hasReservedScope(*ND.getDeclContext())))
+ return false;
+
return true;
}
Index: clang-tools-extra/clangd/index/FileIndex.cpp
===================================================================
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ clang-tools-extra/clangd/index/FileIndex.cpp
@@ -56,6 +56,9 @@
CollectorOpts.CountReferences = false;
CollectorOpts.Origin = SymbolOrigin::Dynamic;
CollectorOpts.CollectMainFileRefs = CollectMainFileRefs;
+ // We want stdlib implementation details in the index only if we've opened the
+ // file in question. This does means xrefs won't work, though.
+ CollectorOpts.CollectReserved = IsIndexMainAST;
index::IndexingOptions IndexOpts;
// We only need declarations, because we don't count references.
Index: clang-tools-extra/clangd/SourceCode.h
===================================================================
--- clang-tools-extra/clangd/SourceCode.h
+++ clang-tools-extra/clangd/SourceCode.h
@@ -16,6 +16,7 @@
#include "Protocol.h"
#include "support/Context.h"
#include "support/ThreadsafeFS.h"
+#include "clang/Basic/CharInfo.h"
#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/LangOptions.h"
#include "clang/Basic/SourceLocation.h"
@@ -27,7 +28,6 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSet.h"
#include "llvm/Support/Error.h"
-#include "llvm/Support/SHA1.h"
#include <string>
namespace clang {
@@ -330,6 +330,13 @@
bool isSelfContainedHeader(const FileEntry *FE, FileID ID,
const SourceManager &SM, HeaderSearch &HeaderInfo);
+/// Returns true if Name is reserved, like _Foo or __Vector_base.
+inline bool isReservedName(llvm::StringRef Name) {
+ // This doesn't catch all cases, but the most common.
+ return Name.size() >= 2 && Name[0] == '_' &&
+ (isUppercase(Name[1]) || Name[1] == '_');
+}
+
} // namespace clangd
} // namespace clang
#endif
Index: clang-tools-extra/clangd/Quality.cpp
===================================================================
--- clang-tools-extra/clangd/Quality.cpp
+++ clang-tools-extra/clangd/Quality.cpp
@@ -24,7 +24,6 @@
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/FormatVariadic.h"
@@ -35,11 +34,6 @@
namespace clang {
namespace clangd {
-static bool isReserved(llvm::StringRef Name) {
- // FIXME: Should we exclude _Bool and others recognized by the standard?
- return Name.size() >= 2 && Name[0] == '_' &&
- (isUppercase(Name[1]) || Name[1] == '_');
-}
static bool hasDeclInMainFile(const Decl &D) {
auto &SourceMgr = D.getASTContext().getSourceManager();
@@ -188,9 +182,10 @@
if (SemaCCResult.Declaration) {
ImplementationDetail |= isImplementationDetail(SemaCCResult.Declaration);
if (auto *ID = SemaCCResult.Declaration->getIdentifier())
- ReservedName = ReservedName || isReserved(ID->getName());
+ ReservedName = ReservedName || isReservedName(ID->getName());
} else if (SemaCCResult.Kind == CodeCompletionResult::RK_Macro)
- ReservedName = ReservedName || isReserved(SemaCCResult.Macro->getName());
+ ReservedName =
+ ReservedName || isReservedName(SemaCCResult.Macro->getName());
}
void SymbolQualitySignals::merge(const Symbol &IndexResult) {
@@ -198,7 +193,7 @@
ImplementationDetail |= (IndexResult.Flags & Symbol::ImplementationDetail);
References = std::max(IndexResult.References, References);
Category = categorize(IndexResult.SymInfo);
- ReservedName = ReservedName || isReserved(IndexResult.Name);
+ ReservedName = ReservedName || isReservedName(IndexResult.Name);
}
float SymbolQualitySignals::evaluateHeuristics() const {
Index: clang-tools-extra/clangd/AST.h
===================================================================
--- clang-tools-extra/clangd/AST.h
+++ clang-tools-extra/clangd/AST.h
@@ -74,6 +74,12 @@
// `MyClass()`, `MyClass(Category)`, and `MyProtocol`.
std::string printObjCContainer(const ObjCContainerDecl &C);
+/// Returns true if this is a NamedDecl with a reserved name.
+bool hasReservedName(const Decl &);
+/// Returns true if this scope would be written with a reserved name.
+/// This does not include unwritten scope elements like __1 in std::__1::vector.
+bool hasReservedScope(const DeclContext &);
+
/// Gets the symbol ID for a declaration. Returned SymbolID might be null.
SymbolID getSymbolID(const Decl *D);
Index: clang-tools-extra/clangd/AST.cpp
===================================================================
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -376,6 +376,24 @@
return OS.str();
}
+bool hasReservedName(const Decl &D) {
+ if (const auto *ND = llvm::dyn_cast<NamedDecl>(&D))
+ if (const auto *II = ND->getIdentifier())
+ return isReservedName(II->getName());
+ return false;
+}
+
+bool hasReservedScope(const DeclContext &DC) {
+ for (const DeclContext *D = &DC; D; D = D->getParent()) {
+ if (D->isTransparentContext() || D->isInlineNamespace())
+ continue;
+ if (const auto *ND = llvm::dyn_cast<NamedDecl>(D))
+ if (hasReservedName(*ND))
+ return true;
+ }
+ return false;
+}
+
QualType declaredType(const TypeDecl *D) {
if (const auto *CTSD = llvm::dyn_cast<ClassTemplateSpecializationDecl>(D))
if (const auto *TSI = CTSD->getTypeAsWritten())
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
