[clangd] Change index scope convention from "outer::inner" to "outer::inner::"

Global scope is "" (was "")
Top-level namespace scope is "ns::" (was "ns")
Nested namespace scope is "ns::ns::" (was "ns::ns")

This composes more naturally:
- qname = scope + name
- full scope = resolved scope + unresolved scope (D42073 was the trigger)
It removes a wart from the old way: "foo::" has one more separator than "".

Another alternative that has these properties is "::ns", but that lacks
the property that both the scope and the name are substrings of the
qname as produced by clang.

This is re-landing r322996 which didn't build.

llvm-svn: 323000

diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index 1a95b6b..639547d 100644 (file)
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -313,7 +313,7 @@ llvm::Optional<SymbolID> getSymbolID(const CodeCompletionResult &R) {
 /// completion (e.g. "ns::ab?").
 struct SpecifiedScope {
   /// The scope specifier as written. For example, for completion "ns::ab?", the
-  /// written scope specifier is "ns".
+  /// written scope specifier is "ns::".
   std::string Written;
   // If this scope specifier is recognized in Sema (e.g. as a namespace
   // context), this will be set to the fully qualfied name of the corresponding
@@ -321,8 +321,8 @@ struct SpecifiedScope {
   std::string Resolved;
 
   llvm::StringRef forIndex() {
-    llvm::StringRef Chosen = Resolved.empty() ? Written : Resolved;
-    return Chosen.trim(':');
+    return Resolved.empty() ? StringRef(Written).ltrim("::")
+                            : StringRef(Resolved);
   }
 };
 
@@ -631,15 +631,14 @@ SpecifiedScope getSpecifiedScope(Sema &S, const CXXScopeSpec &SS) {
   auto SpecifierRange = SS.getRange();
   Info.Written = Lexer::getSourceText(
       CharSourceRange::getCharRange(SpecifierRange), SM, clang::LangOptions());
+  if (!Info.Written.empty())
+    Info.Written += "::"; // Sema excludes the trailing ::.
   if (SS.isValid()) {
     DeclContext *DC = S.computeDeclContext(SS);
     if (auto *NS = llvm::dyn_cast<NamespaceDecl>(DC)) {
-      Info.Resolved = NS->getQualifiedNameAsString();
+      Info.Resolved = NS->getQualifiedNameAsString() + "::";
     } else if (llvm::dyn_cast<TranslationUnitDecl>(DC) != nullptr) {
-      Info.Resolved = "::";
-      // Sema does not include the suffix "::" in the range of SS, so we add
-      // it back here.
-      Info.Written = "::";
+      Info.Resolved = "";
     }
   }
   return Info;

diff --git a/clang-tools-extra/clangd/index/Index.h b/clang-tools-extra/clangd/index/Index.h
index a70d38b..a3eb298 100644 (file)
--- a/clang-tools-extra/clangd/index/Index.h
+++ b/clang-tools-extra/clangd/index/Index.h
@@ -114,10 +114,9 @@ struct Symbol {
   SymbolID ID;
   // The symbol information, like symbol kind.
   index::SymbolInfo SymInfo;
-  // The unqualified name of the symbol, e.g. "bar" (for "n1::n2::bar").
+  // The unqualified name of the symbol, e.g. "bar" (for ns::bar).
   llvm::StringRef Name;
-  // The scope (e.g. namespace) of the symbol, e.g. "n1::n2" (for
-  // "n1::n2::bar").
+  // The containing namespace. e.g. "" (global), "ns::" (top-level namespace).
   llvm::StringRef Scope;
   // The location of the canonical declaration of the symbol.
   //
@@ -221,12 +220,11 @@ struct FuzzyFindRequest {
   /// un-qualified identifiers and should not contain qualifiers like "::".
   std::string Query;
   /// \brief If this is non-empty, symbols must be in at least one of the scopes
-  /// (e.g. namespaces) excluding nested scopes. For example, if a scope "xyz"
-  /// is provided, the matched symbols must be defined in scope "xyz" but not
-  /// "xyz::abc".
+  /// (e.g. namespaces) excluding nested scopes. For example, if a scope "xyz::"
+  /// is provided, the matched symbols must be defined in namespace xyz but not
+  /// namespace xyz::abc.
   ///
-  /// A scope must be fully qualified without leading or trailing "::" e.g.
-  /// "n1::n2". "" is interpreted as the global namespace, and "::" is invalid.
+  /// The global scope is "", a top level scope is "foo::", etc.
   std::vector<std::string> Scopes;
   /// \brief The number of top candidates to return. The index may choose to
   /// return more than this, e.g. if it doesn't know which candidates are best.

diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index 92402c5..e7fae17 100644 (file)
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -56,14 +56,14 @@ std::string makeAbsolutePath(const SourceManager &SM, StringRef Path) {
   return AbsolutePath.str();
 }
 
-// "a::b::c", return {"a::b", "c"}. Scope is empty if it doesn't exist.
+// "a::b::c", return {"a::b::", "c"}. Scope is empty if there's no qualifier.
 std::pair<llvm::StringRef, llvm::StringRef>
 splitQualifiedName(llvm::StringRef QName) {
   assert(!QName.startswith("::") && "Qualified names should not start with ::");
   size_t Pos = QName.rfind("::");
   if (Pos == llvm::StringRef::npos)
     return {StringRef(), QName};
-  return {QName.substr(0, Pos), QName.substr(Pos + 2)};
+  return {QName.substr(0, Pos + 2), QName.substr(Pos + 2)};
 }
 
 bool shouldFilterDecl(const NamedDecl *ND, ASTContext *ASTCtx,
@@ -147,12 +147,10 @@ bool SymbolCollector::handleDeclOccurence(
     SymbolLocation Location = {FilePath, SM.getFileOffset(D->getLocStart()),
                                SM.getFileOffset(D->getLocEnd())};
     std::string QName = ND->getQualifiedNameAsString();
-    auto ScopeAndName = splitQualifiedName(QName);
 
     Symbol S;
     S.ID = std::move(ID);
-    S.Scope = ScopeAndName.first;
-    S.Name = ScopeAndName.second;
+    std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
     S.SymInfo = index::getSymbolInfo(D);
     S.CanonicalDeclaration = Location;
 

diff --git a/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp b/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
index 7eb7b13..4000550 100644 (file)
--- a/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
+++ b/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
@@ -155,8 +155,8 @@ Symbol sym(StringRef QName, index::SymbolKind Kind, StringRef USRFormat) {
     Sym.Scope = "";
   } else {
     Sym.Name = QName.substr(Pos + 2);
-    Sym.Scope = QName.substr(0, Pos);
-    USR += "@N@" + replace(Sym.Scope, "::", "@N@"); // ns:: -> @N@ns
+    Sym.Scope = QName.substr(0, Pos + 2);
+    USR += "@N@" + replace(QName.substr(0, Pos), "::", "@N@"); // ns:: -> @N@ns
   }
   USR += Regex("^.*$").sub(USRFormat, Sym.Name); // e.g. func -> @F@func#
   Sym.ID = SymbolID(USR);

diff --git a/clang-tools-extra/unittests/clangd/FileIndexTests.cpp b/clang-tools-extra/unittests/clangd/FileIndexTests.cpp
index adae1f1..85352bc 100644 (file)
--- a/clang-tools-extra/unittests/clangd/FileIndexTests.cpp
+++ b/clang-tools-extra/unittests/clangd/FileIndexTests.cpp
@@ -78,8 +78,7 @@ std::vector<std::string> match(const SymbolIndex &I,
   std::vector<std::string> Matches;
   auto Ctx = Context::empty();
   I.fuzzyFind(Ctx, Req, [&](const Symbol &Sym) {
-    Matches.push_back(
-        (Sym.Scope + (Sym.Scope.empty() ? "" : "::") + Sym.Name).str());
+    Matches.push_back((Sym.Scope + Sym.Name).str());
   });
   return Matches;
 }
@@ -110,7 +109,7 @@ TEST(FileIndexTest, IndexAST) {
 
   FuzzyFindRequest Req;
   Req.Query = "";
-  Req.Scopes = {"ns"};
+  Req.Scopes = {"ns::"};
   EXPECT_THAT(match(M, Req), UnorderedElementsAre("ns::f", "ns::X"));
 }
 
@@ -139,7 +138,7 @@ TEST(FileIndexTest, IndexMultiASTAndDeduplicate) {
 
   FuzzyFindRequest Req;
   Req.Query = "";
-  Req.Scopes = {"ns"};
+  Req.Scopes = {"ns::"};
   EXPECT_THAT(match(M, Req), UnorderedElementsAre("ns::f", "ns::X", "ns::ff"));
 }
 
@@ -152,7 +151,7 @@ TEST(FileIndexTest, RemoveAST) {
 
   FuzzyFindRequest Req;
   Req.Query = "";
-  Req.Scopes = {"ns"};
+  Req.Scopes = {"ns::"};
   EXPECT_THAT(match(M, Req), UnorderedElementsAre("ns::f", "ns::X"));
 
   M.update(Ctx, "f1", nullptr);

diff --git a/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp b/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
index 8793257..c17d535 100644 (file)
--- a/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
@@ -43,9 +43,7 @@ MATCHER_P(Plain, Text, "") { return arg.CompletionPlainInsertText == Text; }
 MATCHER_P(Snippet, S, "") {
   return arg.CompletionSnippetInsertText == S;
 }
-MATCHER_P(QName, Name, "") {
-  return (arg.Scope + (arg.Scope.empty() ? "" : "::") + arg.Name).str() == Name;
-}
+MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
 
 namespace clang {
 namespace clangd {
@@ -291,7 +289,7 @@ TEST_F(SymbolCollectorTest, YAMLConversions) {
 ---
 ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF856
 Name:   'Foo1'
-Scope:   'clang'
+Scope:   'clang::'
 SymInfo:
   Kind:            Function
   Lang:            Cpp
@@ -311,7 +309,7 @@ Detail:
 ---
 ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF858
 Name:   'Foo2'
-Scope:   'clang'
+Scope:   'clang::'
 SymInfo:
   Kind:            Function
   Lang:            Cpp