[clang-tools-extra] r269430 - [include-fixer] Use scope contexts information to improve query.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Fri May 13 10:44:16 2016
New Revision: 269430

URL: http://llvm.org/viewvc/llvm-project?rev=269430&view=rev
Log:
[include-fixer] Use scope contexts information to improve query.

Reviewers: bkramer

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D20205

Modified:
    clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp
    clang-tools-extra/trunk/include-fixer/SymbolIndexManager.cpp
    clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp

Modified: clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp?rev=269430&r1=269429&r2=269430&view=diff
==============================================================================
--- clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp (original)
+++ clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp Fri May 13 10:44:16 2016
@@ -116,6 +116,19 @@ public:
     if (getCompilerInstance().getSema().isSFINAEContext())
       return clang::TypoCorrection();
 
+    std::string TypoScopeString;
+    if (S) {
+      // FIXME: Currently we only use namespace contexts. Use other context
+      // types for query.
+      for (const auto *Context = S->getEntity(); Context;
+           Context = Context->getParent()) {
+        if (const auto *ND = dyn_cast<NamespaceDecl>(Context)) {
+          if (!ND->getName().empty())
+            TypoScopeString = ND->getNameAsString() + "::" + TypoScopeString;
+        }
+      }
+    }
+
     /// If we have a scope specification, use that to get more precise results.
     std::string QueryString;
     if (SS && SS->getRange().isValid()) {
@@ -150,7 +163,23 @@ public:
       QueryString = Typo.getAsString();
     }
 
-    return query(QueryString, Typo.getLoc());
+    // Follow C++ Lookup rules. Firstly, lookup the identifier with scoped
+    // namespace contexts. If fails, falls back to identifier.
+    // For example:
+    //
+    // namespace a {
+    // b::foo f;
+    // }
+    //
+    // 1. lookup a::b::foo.
+    // 2. lookup b::foo.
+    if (!query(TypoScopeString + QueryString, Typo.getLoc()))
+      query(QueryString, Typo.getLoc());
+
+    // FIXME: We should just return the name we got as input here and prevent
+    // clang from trying to correct the typo by itself. That may change the
+    // identifier to something that's not wanted by the user.
+    return clang::TypoCorrection();
   }
 
   StringRef filename() const { return Filename; }
@@ -235,12 +264,12 @@ public:
 
 private:
   /// Query the database for a given identifier.
-  clang::TypoCorrection query(StringRef Query, SourceLocation Loc) {
+  bool query(StringRef Query, SourceLocation Loc) {
     assert(!Query.empty() && "Empty query!");
 
     // Save database lookups by not looking up identifiers multiple times.
     if (!SeenQueries.insert(Query).second)
-      return clang::TypoCorrection();
+      return true;
 
     DEBUG(llvm::dbgs() << "Looking up '" << Query << "' at ");
     DEBUG(Loc.print(llvm::dbgs(), getCompilerInstance().getSourceManager()));
@@ -250,16 +279,13 @@ private:
     auto SearchReply = SymbolIndexMgr.search(Query);
     DEBUG(llvm::dbgs() << SearchReply.size() << " replies\n");
     if (SearchReply.empty())
-      return clang::TypoCorrection();
+      return false;
 
     // Add those files to the set of includes to try out.
     // FIXME: Rank the results and pick the best one instead of the first one.
     TryInclude(Query, SearchReply[0]);
 
-    // FIXME: We should just return the name we got as input here and prevent
-    // clang from trying to correct the typo by itself. That may change the
-    // identifier to something that's not wanted by the user.
-    return clang::TypoCorrection();
+    return true;
   }
 
   /// The client to use to find cross-references.

Modified: clang-tools-extra/trunk/include-fixer/SymbolIndexManager.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/include-fixer/SymbolIndexManager.cpp?rev=269430&r1=269429&r2=269430&view=diff
==============================================================================
--- clang-tools-extra/trunk/include-fixer/SymbolIndexManager.cpp (original)
+++ clang-tools-extra/trunk/include-fixer/SymbolIndexManager.cpp Fri May 13 10:44:16 2016
@@ -39,10 +39,10 @@ SymbolIndexManager::search(llvm::StringR
     if (Symbol.getName() == Names.back()) {
       bool IsMatched = true;
       auto SymbolContext = Symbol.getContexts().begin();
+      auto IdentiferContext = Names.rbegin() + 1; // Skip identifier name;
       // Match the remaining context names.
-      for (auto IdentiferContext = Names.rbegin() + 1;
-           IdentiferContext != Names.rend() &&
-           SymbolContext != Symbol.getContexts().end();
+      for (; IdentiferContext != Names.rend() &&
+             SymbolContext != Symbol.getContexts().end();
            ++IdentiferContext, ++SymbolContext) {
         if (SymbolContext->second != *IdentiferContext) {
           IsMatched = false;
@@ -50,7 +50,9 @@ SymbolIndexManager::search(llvm::StringR
         }
       }
 
-      if (IsMatched) {
+      // FIXME: Support full match. At this point, we only find symbols in
+      // database which end with the same contexts with the identifier.
+      if (IsMatched && IdentiferContext == Names.rend()) {
         // FIXME: file path should never be in the form of <...> or "...", but
         // the unit test with fixed database use <...> file path, which might
         // need to be changed.

Modified: clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp?rev=269430&r1=269429&r2=269430&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp (original)
+++ clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp Fri May 13 10:44:16 2016
@@ -59,6 +59,9 @@ static std::string runIncludeFixer(
       SymbolInfo("foo", SymbolInfo::SymbolKind::Class, "\"dir/otherdir/qux.h\"",
                  1, {{SymbolInfo::ContextType::Namespace, "b"},
                      {SymbolInfo::ContextType::Namespace, "a"}}),
+      SymbolInfo("bar", SymbolInfo::SymbolKind::Class, "\"bar.h\"",
+                 1, {{SymbolInfo::ContextType::Namespace, "b"},
+                     {SymbolInfo::ContextType::Namespace, "a"}}),
   };
   auto SymbolIndexMgr = llvm::make_unique<include_fixer::SymbolIndexManager>();
   SymbolIndexMgr->addSymbolIndex(
@@ -137,6 +140,20 @@ TEST(IncludeFixer, MultipleMissingSymbol
             runIncludeFixer("std::string bar;\nstd::sting foo;\n"));
 }
 
+TEST(IncludeFixer, ScopedNamespaceSymbols) {
+  EXPECT_EQ("#include \"bar.h\"\nnamespace a { b::bar b; }\n",
+            runIncludeFixer("namespace a { b::bar b; }\n"));
+  EXPECT_EQ("#include \"bar.h\"\nnamespace A { a::b::bar b; }\n",
+            runIncludeFixer("namespace A { a::b::bar b; }\n"));
+  EXPECT_EQ("#include \"bar.h\"\nnamespace a { void func() { b::bar b; } }\n",
+            runIncludeFixer("namespace a { void func() { b::bar b; } }\n"));
+  EXPECT_EQ("namespace A { c::b::bar b; }\n",
+            runIncludeFixer("namespace A { c::b::bar b; }\n"));
+  // FIXME: The header should not be added here. Remove this after we support
+  // full match.
+  EXPECT_EQ("#include \"bar.h\"\nnamespace A { b::bar b; }\n",
+            runIncludeFixer("namespace A { b::bar b; }\n"));
+}
 } // namespace
 } // namespace include_fixer
 } // namespace clang