[include-fixer] only deduplicate symbols after matching symbols with the undefined identifier.

Summary:
we should only deduplicate symbols after matching symbols with the
undefined identifier. This patch tries to fix the situation where matching
symbols are deleted when it has the same header as a non-matching symbol, which
can prevent finding the correct header even if there is a matched symbol.

Reviewers: bkramer

Subscribers: cfe-commits

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

llvm-svn: 272576

2 files changed, 36 insertions, 20 deletions

diff --git a/clang-tools-extra/include-fixer/SymbolIndexManager.cpp b/clang-tools-extra/include-fixer/SymbolIndexManager.cpp
index b3ecca49289..ae9faa28523 100644
--- a/clang-tools-extra/include-fixer/SymbolIndexManager.cpp
+++ b/clang-tools-extra/include-fixer/SymbolIndexManager.cpp
@@ -67,8 +67,8 @@ SymbolIndexManager::search(llvm::StringRef Identifier) const {
   // Eventually we will either hit a class (namespaces aren't in the database
   // either) and can report that result.
   bool TookPrefix = false;
-  std::vector<std::string> Results;
-  while (Results.empty() && !Names.empty()) {
+  std::vector<clang::find_all_symbols::SymbolInfo> MatchedSymbols;
+  while (MatchedSymbols.empty() && !Names.empty()) {
     std::vector<clang::find_all_symbols::SymbolInfo> Symbols;
     for (const auto &DB : SymbolIndices) {
       auto Res = DB->search(Names.back().str());
@@ -78,8 +78,6 @@ SymbolIndexManager::search(llvm::StringRef Identifier) const {
     DEBUG(llvm::dbgs() << "Searching " << Names.back() << "... got "
                        << Symbols.size() << " results...\n");
 
-    rankByPopularity(Symbols);
-
     for (const auto &Symbol : Symbols) {
       // Match the identifier name without qualifier.
       if (Symbol.getName() == Names.back()) {
@@ -120,15 +118,7 @@ SymbolIndexManager::search(llvm::StringRef Identifier) const {
                Symbol.getSymbolKind() == SymbolInfo::SymbolKind::Macro))
             continue;
 
-          // 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.
-          // FIXME: if the file path is a system header name, we want to use
-          // angle brackets.
-          std::string FilePath = Symbol.getFilePath().str();
-          Results.push_back((FilePath[0] == '"' || FilePath[0] == '<')
-                                ? FilePath
-                                : "\"" + FilePath + "\"");
+          MatchedSymbols.push_back(Symbol);
         }
       }
     }
@@ -136,6 +126,20 @@ SymbolIndexManager::search(llvm::StringRef Identifier) const {
     TookPrefix = true;
   }
 
+  rankByPopularity(MatchedSymbols);
+  std::vector<std::string> Results;
+  for (const auto &Symbol : MatchedSymbols) {
+    // 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.
+    // FIXME: if the file path is a system header name, we want to use
+    // angle brackets.
+    std::string FilePath = Symbol.getFilePath().str();
+    Results.push_back((FilePath[0] == '"' || FilePath[0] == '<')
+                          ? FilePath
+                          : "\"" + FilePath + "\"");
+  }
+
   return Results;
 }
 
diff --git a/clang-tools-extra/unittests/include-fixer/IncludeFixerTest.cpp b/clang-tools-extra/unittests/include-fixer/IncludeFixerTest.cpp
index 4b9c117ae05..d0bdccacc7c 100644
--- a/clang-tools-extra/unittests/include-fixer/IncludeFixerTest.cpp
+++ b/clang-tools-extra/unittests/include-fixer/IncludeFixerTest.cpp
@@ -60,13 +60,20 @@ 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"}}),
-      SymbolInfo("Green", SymbolInfo::SymbolKind::Class, "\"color.h\"",
-                 1, {{SymbolInfo::ContextType::EnumDecl, "Color"},
-                     {SymbolInfo::ContextType::Namespace, "b"},
-                     {SymbolInfo::ContextType::Namespace, "a"}}),
+      SymbolInfo("bar", SymbolInfo::SymbolKind::Class, "\"bar.h\"", 1,
+                 {{SymbolInfo::ContextType::Namespace, "b"},
+                  {SymbolInfo::ContextType::Namespace, "a"}}),
+      SymbolInfo("Green", SymbolInfo::SymbolKind::Class, "\"color.h\"", 1,
+                 {{SymbolInfo::ContextType::EnumDecl, "Color"},
+                  {SymbolInfo::ContextType::Namespace, "b"},
+                  {SymbolInfo::ContextType::Namespace, "a"}}),
+      SymbolInfo("Vector", SymbolInfo::SymbolKind::Class, "\"Vector.h\"", 1,
+                 {{SymbolInfo::ContextType::Namespace, "__a"},
+                  {SymbolInfo::ContextType::Namespace, "a"}},
+                 /*num_occurrences=*/2),
+      SymbolInfo("Vector", SymbolInfo::SymbolKind::Class, "\"Vector.h\"", 2,
+                 {{SymbolInfo::ContextType::Namespace, "a"}},
+                 /*num_occurrences=*/1),
   };
   auto SymbolIndexMgr = llvm::make_unique<include_fixer::SymbolIndexManager>();
   SymbolIndexMgr->addSymbolIndex(
@@ -209,6 +216,11 @@ TEST(IncludeFixer, InsertAndSortSingleHeader) {
   EXPECT_EQ(Expected, runIncludeFixer(Code));
 }
 
+TEST(IncludeFixer, DoNotDeleteMatchedSymbol) {
+  EXPECT_EQ("#include \"Vector.h\"\na::Vector v;",
+            runIncludeFixer("a::Vector v;"));
+}
+
 } // namespace
 } // namespace include_fixer
 } // namespace clang