[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL296446: [include-fixer] Add usage count to find-all-symbols. 
(authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D30210?vs=89982=89983#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D30210

Files:
  clang-tools-extra/trunk/include-fixer/InMemorySymbolIndex.cpp
  clang-tools-extra/trunk/include-fixer/InMemorySymbolIndex.h
  clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp
  clang-tools-extra/trunk/include-fixer/SymbolIndex.h
  clang-tools-extra/trunk/include-fixer/SymbolIndexManager.cpp
  clang-tools-extra/trunk/include-fixer/YamlSymbolIndex.cpp
  clang-tools-extra/trunk/include-fixer/YamlSymbolIndex.h
  clang-tools-extra/trunk/include-fixer/find-all-symbols/FindAllMacros.cpp
  clang-tools-extra/trunk/include-fixer/find-all-symbols/FindAllMacros.h
  clang-tools-extra/trunk/include-fixer/find-all-symbols/FindAllSymbols.cpp
  clang-tools-extra/trunk/include-fixer/find-all-symbols/FindAllSymbols.h
  
clang-tools-extra/trunk/include-fixer/find-all-symbols/FindAllSymbolsAction.cpp
  clang-tools-extra/trunk/include-fixer/find-all-symbols/SymbolInfo.cpp
  clang-tools-extra/trunk/include-fixer/find-all-symbols/SymbolInfo.h
  clang-tools-extra/trunk/include-fixer/find-all-symbols/SymbolReporter.h
  
clang-tools-extra/trunk/include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp
  clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp
  clang-tools-extra/trunk/test/include-fixer/Inputs/fake_yaml_db.yaml
  clang-tools-extra/trunk/test/include-fixer/Inputs/merge/a.yaml
  clang-tools-extra/trunk/test/include-fixer/Inputs/merge/b.yaml
  clang-tools-extra/trunk/test/include-fixer/merge.test
  clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp
  
clang-tools-extra/trunk/unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

Index: clang-tools-extra/trunk/test/include-fixer/merge.test
===
--- clang-tools-extra/trunk/test/include-fixer/merge.test
+++ clang-tools-extra/trunk/test/include-fixer/merge.test
@@ -9,7 +9,8 @@
 FilePath:../include/bar.h
 LineNumber:  1
 Type:Class
-NumOccurrences:  1
+Seen:1
+Used:1
 ...
 ---
 Name:bar
@@ -19,7 +20,8 @@
 FilePath:../include/barbar.h
 LineNumber:  1
 Type:Class
-NumOccurrences:  1
+Seen:1
+Used:0
 ...
 ---
 Name:foo
@@ -29,5 +31,6 @@
 FilePath:foo.h
 LineNumber:  1
 Type:Class
-NumOccurrences:  2
+Seen:2
+Used:2
 ...
Index: clang-tools-extra/trunk/test/include-fixer/Inputs/merge/b.yaml
===
--- clang-tools-extra/trunk/test/include-fixer/Inputs/merge/b.yaml
+++ clang-tools-extra/trunk/test/include-fixer/Inputs/merge/b.yaml
@@ -6,7 +6,8 @@
 FilePath:foo.h
 LineNumber:  1
 Type:Class
-NumOccurrences:  1
+Seen:1
+Used:2
 ...
 ---
 Name:   bar
@@ -16,5 +17,6 @@
 FilePath:../include/barbar.h
 LineNumber:  1
 Type:Class
-NumOccurrences:  1
+Seen:1
+Used:0
 ...
Index: clang-tools-extra/trunk/test/include-fixer/Inputs/merge/a.yaml
===
--- clang-tools-extra/trunk/test/include-fixer/Inputs/merge/a.yaml
+++ clang-tools-extra/trunk/test/include-fixer/Inputs/merge/a.yaml
@@ -6,7 +6,8 @@
 FilePath:foo.h
 LineNumber:  1
 Type:Class
-NumOccurrences:  1
+Seen:1
+Used:1
 ...
 ---
 Name:   bar
@@ -16,5 +17,6 @@
 FilePath:../include/bar.h
 LineNumber:  1
 Type:Class
-NumOccurrences:  1
+Seen:1
+Used:2
 ...
Index: clang-tools-extra/trunk/test/include-fixer/Inputs/fake_yaml_db.yaml
===
--- clang-tools-extra/trunk/test/include-fixer/Inputs/fake_yaml_db.yaml
+++ clang-tools-extra/trunk/test/include-fixer/Inputs/fake_yaml_db.yaml
@@ -8,7 +8,8 @@
 FilePath:foo.h
 LineNumber:  1
 Type:Class
-NumOccurrences:  1
+Seen:1
+Used:0
 ---
 Name:   bar
 Contexts:
@@ -19,7 +20,8 @@
 FilePath:../include/bar.h
 LineNumber:  1
 Type:Class
-NumOccurrences:  1
+Seen:1
+Used:0
 ---
 Name:   bar
 Contexts:
@@ -30,7 +32,8 @@
 FilePath:../include/bar.h
 LineNumber:  2
 Type:Class
-NumOccurrences:  3
+Seen:3
+Used:0
 ---
 Name:   bar
 Contexts:
@@ -41,20 +44,23 @@
 FilePath:../include/zbar.h
 LineNumber:  1
 Type:Class
-NumOccurrences:  3
+Seen:3
+Used:0
 ---
 Name:   b
 Contexts:
 FilePath:var.h
 LineNumber: 

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 89982.
sammccall marked an inline comment as done.
sammccall added a comment.

Review comments


https://reviews.llvm.org/D30210

Files:
  include-fixer/InMemorySymbolIndex.cpp
  include-fixer/InMemorySymbolIndex.h
  include-fixer/IncludeFixer.cpp
  include-fixer/SymbolIndex.h
  include-fixer/SymbolIndexManager.cpp
  include-fixer/YamlSymbolIndex.cpp
  include-fixer/YamlSymbolIndex.h
  include-fixer/find-all-symbols/FindAllMacros.cpp
  include-fixer/find-all-symbols/FindAllMacros.h
  include-fixer/find-all-symbols/FindAllSymbols.cpp
  include-fixer/find-all-symbols/FindAllSymbols.h
  include-fixer/find-all-symbols/FindAllSymbolsAction.cpp
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/find-all-symbols/SymbolReporter.h
  include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp
  include-fixer/tool/ClangIncludeFixer.cpp
  test/include-fixer/Inputs/fake_yaml_db.yaml
  test/include-fixer/Inputs/merge/a.yaml
  test/include-fixer/Inputs/merge/b.yaml
  test/include-fixer/merge.test
  unittests/include-fixer/IncludeFixerTest.cpp
  unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

Index: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
===
--- unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
+++ unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
@@ -19,6 +19,7 @@
 #include "clang/Frontend/PCHContainerOperations.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 "gtest/gtest.h"
@@ -31,34 +32,39 @@
 
 static const char HeaderName[] = "symbols.h";
 
-class TestSymbolReporter : public clang::find_all_symbols::SymbolReporter {
+class TestSymbolReporter : public SymbolReporter {
 public:
   ~TestSymbolReporter() override {}
 
-  void reportSymbol(llvm::StringRef FileName,
-const SymbolInfo ) override {
-Symbols.push_back(Symbol);
+  void reportSymbols(llvm::StringRef FileName,
+ const SymbolInfo::SignalMap ) override {
+for (const auto  : NewSymbols)
+  Symbols[Entry.first] += Entry.second;
   }
 
   bool hasSymbol(const SymbolInfo ) const {
-for (const auto  : Symbols) {
-  if (S == Symbol)
-return true;
-}
-return false;
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Seen > 0;
+  }
+
+  bool hasUse(const SymbolInfo ) const {
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Used > 0;
   }
 
 private:
-  std::vector Symbols;
+  SymbolInfo::SignalMap Symbols;
 };
 
 class FindAllSymbolsTest : public ::testing::Test {
 public:
   bool hasSymbol(const SymbolInfo ) {
 return Reporter.hasSymbol(Symbol);
   }
 
-  bool runFindAllSymbols(StringRef Code) {
+  bool hasUse(const SymbolInfo ) { return Reporter.hasUse(Symbol); }
+
+  bool runFindAllSymbols(StringRef HeaderCode, StringRef MainCode) {
 llvm::IntrusiveRefCntPtr InMemoryFileSystem(
 new vfs::InMemoryFileSystem);
 llvm::IntrusiveRefCntPtr Files(
@@ -88,7 +94,7 @@
 InMemoryFileSystem->addFile(InternalHeader, 0,
 llvm::MemoryBuffer::getMemBuffer(InternalCode));
 
-std::unique_ptr Factory(
+std::unique_ptr Factory(
 new FindAllSymbolsActionFactory(, ));
 
 tooling::ToolInvocation Invocation(
@@ -98,7 +104,7 @@
 std::make_shared());
 
 InMemoryFileSystem->addFile(HeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(Code));
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
 std::string Content = "#include\"" + std::string(HeaderName) +
   "\"\n"
@@ -118,6 +124,7 @@
 SymbolInfo DirtySymbol("ExtraInternal", SymbolInfo::SymbolKind::Class,
CleanHeader, 2, {});
 #endif // _MSC_VER && __MINGW32__
+Content += "\n" + MainCode.str();
 InMemoryFileSystem->addFile(FileName, 0,
 llvm::MemoryBuffer::getMemBuffer(Content));
 Invocation.run();
@@ -135,49 +142,64 @@
 };
 
 TEST_F(FindAllSymbolsTest, VariableSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern int xargc;
   namespace na {
   static bool  = false;
   namespace nb { const long long *; }
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  auto y = ::nb::;
+  int main() { if (na::) return xargc; }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("xargc", SymbolInfo::SymbolKind::Variable, HeaderName, 2, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", 

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Benjamin Kramer via Phabricator via cfe-commits]

bkramer accepted this revision.
bkramer added a comment.

lg




Comment at: include-fixer/SymbolIndexManager.cpp:156
+  for (const auto  : MatchedSymbols)
+Res.push_back(SymAndSig.Symbol);
+  return Res;

std::move



Comment at: include-fixer/find-all-symbols/FindAllMacros.cpp:67
+  FileSymbols);
+  FileSymbols = {};
 }

Call me oldschool, but I still prefer `FileSymbols.clear()`


https://reviews.llvm.org/D30210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 89636.
sammccall added a comment.

Remove redundant std::move


https://reviews.llvm.org/D30210

Files:
  include-fixer/InMemorySymbolIndex.cpp
  include-fixer/InMemorySymbolIndex.h
  include-fixer/IncludeFixer.cpp
  include-fixer/SymbolIndex.h
  include-fixer/SymbolIndexManager.cpp
  include-fixer/YamlSymbolIndex.cpp
  include-fixer/YamlSymbolIndex.h
  include-fixer/find-all-symbols/FindAllMacros.cpp
  include-fixer/find-all-symbols/FindAllMacros.h
  include-fixer/find-all-symbols/FindAllSymbols.cpp
  include-fixer/find-all-symbols/FindAllSymbols.h
  include-fixer/find-all-symbols/FindAllSymbolsAction.cpp
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/find-all-symbols/SymbolReporter.h
  include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp
  include-fixer/tool/ClangIncludeFixer.cpp
  test/include-fixer/Inputs/fake_yaml_db.yaml
  test/include-fixer/Inputs/merge/a.yaml
  test/include-fixer/Inputs/merge/b.yaml
  test/include-fixer/merge.test
  unittests/include-fixer/IncludeFixerTest.cpp
  unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

Index: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
===
--- unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
+++ unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
@@ -19,6 +19,7 @@
 #include "clang/Frontend/PCHContainerOperations.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 "gtest/gtest.h"
@@ -31,34 +32,39 @@
 
 static const char HeaderName[] = "symbols.h";
 
-class TestSymbolReporter : public clang::find_all_symbols::SymbolReporter {
+class TestSymbolReporter : public SymbolReporter {
 public:
   ~TestSymbolReporter() override {}
 
-  void reportSymbol(llvm::StringRef FileName,
-const SymbolInfo ) override {
-Symbols.push_back(Symbol);
+  void reportSymbols(llvm::StringRef FileName,
+ const SymbolInfo::SignalMap ) override {
+for (const auto  : NewSymbols)
+  Symbols[Entry.first] += Entry.second;
   }
 
   bool hasSymbol(const SymbolInfo ) const {
-for (const auto  : Symbols) {
-  if (S == Symbol)
-return true;
-}
-return false;
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Seen > 0;
+  }
+
+  bool hasUse(const SymbolInfo ) const {
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Used > 0;
   }
 
 private:
-  std::vector Symbols;
+  SymbolInfo::SignalMap Symbols;
 };
 
 class FindAllSymbolsTest : public ::testing::Test {
 public:
   bool hasSymbol(const SymbolInfo ) {
 return Reporter.hasSymbol(Symbol);
   }
 
-  bool runFindAllSymbols(StringRef Code) {
+  bool hasUse(const SymbolInfo ) { return Reporter.hasUse(Symbol); }
+
+  bool runFindAllSymbols(StringRef HeaderCode, StringRef MainCode) {
 llvm::IntrusiveRefCntPtr InMemoryFileSystem(
 new vfs::InMemoryFileSystem);
 llvm::IntrusiveRefCntPtr Files(
@@ -88,7 +94,7 @@
 InMemoryFileSystem->addFile(InternalHeader, 0,
 llvm::MemoryBuffer::getMemBuffer(InternalCode));
 
-std::unique_ptr Factory(
+std::unique_ptr Factory(
 new FindAllSymbolsActionFactory(, ));
 
 tooling::ToolInvocation Invocation(
@@ -98,7 +104,7 @@
 std::make_shared());
 
 InMemoryFileSystem->addFile(HeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(Code));
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
 std::string Content = "#include\"" + std::string(HeaderName) +
   "\"\n"
@@ -118,6 +124,7 @@
 SymbolInfo DirtySymbol("ExtraInternal", SymbolInfo::SymbolKind::Class,
CleanHeader, 2, {});
 #endif // _MSC_VER && __MINGW32__
+Content += "\n" + MainCode.str();
 InMemoryFileSystem->addFile(FileName, 0,
 llvm::MemoryBuffer::getMemBuffer(Content));
 Invocation.run();
@@ -135,49 +142,64 @@
 };
 
 TEST_F(FindAllSymbolsTest, VariableSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern int xargc;
   namespace na {
   static bool  = false;
   namespace nb { const long long *; }
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  auto y = ::nb::;
+  int main() { if (na::) return xargc; }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("xargc", SymbolInfo::SymbolKind::Variable, HeaderName, 2, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 4,
   

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks, still LGTM with one nit.




Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:262
+  if (Filename != "") {
+Reporter->reportSymbols(Filename, std::move(FileSymbols));
+FileSymbols = {};

We don't need `std::move` right now.


https://reviews.llvm.org/D30210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 89632.
sammccall added a comment.

Report symbols by reference.


https://reviews.llvm.org/D30210

Files:
  include-fixer/InMemorySymbolIndex.cpp
  include-fixer/InMemorySymbolIndex.h
  include-fixer/IncludeFixer.cpp
  include-fixer/SymbolIndex.h
  include-fixer/SymbolIndexManager.cpp
  include-fixer/YamlSymbolIndex.cpp
  include-fixer/YamlSymbolIndex.h
  include-fixer/find-all-symbols/FindAllMacros.cpp
  include-fixer/find-all-symbols/FindAllMacros.h
  include-fixer/find-all-symbols/FindAllSymbols.cpp
  include-fixer/find-all-symbols/FindAllSymbols.h
  include-fixer/find-all-symbols/FindAllSymbolsAction.cpp
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/find-all-symbols/SymbolReporter.h
  include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp
  include-fixer/tool/ClangIncludeFixer.cpp
  test/include-fixer/Inputs/fake_yaml_db.yaml
  test/include-fixer/Inputs/merge/a.yaml
  test/include-fixer/Inputs/merge/b.yaml
  test/include-fixer/merge.test
  unittests/include-fixer/IncludeFixerTest.cpp
  unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

Index: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
===
--- unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
+++ unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
@@ -19,6 +19,7 @@
 #include "clang/Frontend/PCHContainerOperations.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 "gtest/gtest.h"
@@ -31,34 +32,39 @@
 
 static const char HeaderName[] = "symbols.h";
 
-class TestSymbolReporter : public clang::find_all_symbols::SymbolReporter {
+class TestSymbolReporter : public SymbolReporter {
 public:
   ~TestSymbolReporter() override {}
 
-  void reportSymbol(llvm::StringRef FileName,
-const SymbolInfo ) override {
-Symbols.push_back(Symbol);
+  void reportSymbols(llvm::StringRef FileName,
+ const SymbolInfo::SignalMap ) override {
+for (const auto  : NewSymbols)
+  Symbols[Entry.first] += Entry.second;
   }
 
   bool hasSymbol(const SymbolInfo ) const {
-for (const auto  : Symbols) {
-  if (S == Symbol)
-return true;
-}
-return false;
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Seen > 0;
+  }
+
+  bool hasUse(const SymbolInfo ) const {
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Used > 0;
   }
 
 private:
-  std::vector Symbols;
+  SymbolInfo::SignalMap Symbols;
 };
 
 class FindAllSymbolsTest : public ::testing::Test {
 public:
   bool hasSymbol(const SymbolInfo ) {
 return Reporter.hasSymbol(Symbol);
   }
 
-  bool runFindAllSymbols(StringRef Code) {
+  bool hasUse(const SymbolInfo ) { return Reporter.hasUse(Symbol); }
+
+  bool runFindAllSymbols(StringRef HeaderCode, StringRef MainCode) {
 llvm::IntrusiveRefCntPtr InMemoryFileSystem(
 new vfs::InMemoryFileSystem);
 llvm::IntrusiveRefCntPtr Files(
@@ -88,7 +94,7 @@
 InMemoryFileSystem->addFile(InternalHeader, 0,
 llvm::MemoryBuffer::getMemBuffer(InternalCode));
 
-std::unique_ptr Factory(
+std::unique_ptr Factory(
 new FindAllSymbolsActionFactory(, ));
 
 tooling::ToolInvocation Invocation(
@@ -98,7 +104,7 @@
 std::make_shared());
 
 InMemoryFileSystem->addFile(HeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(Code));
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
 std::string Content = "#include\"" + std::string(HeaderName) +
   "\"\n"
@@ -118,6 +124,7 @@
 SymbolInfo DirtySymbol("ExtraInternal", SymbolInfo::SymbolKind::Class,
CleanHeader, 2, {});
 #endif // _MSC_VER && __MINGW32__
+Content += "\n" + MainCode.str();
 InMemoryFileSystem->addFile(FileName, 0,
 llvm::MemoryBuffer::getMemBuffer(Content));
 Invocation.run();
@@ -135,49 +142,64 @@
 };
 
 TEST_F(FindAllSymbolsTest, VariableSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern int xargc;
   namespace na {
   static bool  = false;
   namespace nb { const long long *; }
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  auto y = ::nb::;
+  int main() { if (na::) return xargc; }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("xargc", SymbolInfo::SymbolKind::Variable, HeaderName, 2, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 4,
 

Re: [PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via cfe-commits]

That's two votes for "this is too surprising" - changed.

https://imgflip.com/i/1k93rm
https://68.media.tumblr.com/8db2fe0a6f84ff128157a2b615f519bf/tumblr_inline_nenq4hMoQA1sb080b.gif

On Fri, Feb 24, 2017 at 9:54 AM, Manuel Klimek  wrote:

> On Thu, Feb 23, 2017 at 10:40 PM Sam McCall  wrote:
>
>>
>>
>> On Feb 23, 2017 8:48 PM, "Haojian Wu via Phabricator" <
>> revi...@reviews.llvm.org> wrote:
>>
>> hokein added inline comments.
>>
>>
>> 
>> Comment at: unittests/include-fixer/find-all-symbols/
>> FindAllSymbolsTests.cpp:40
>> +  void reportSymbols(llvm::StringRef FileName,
>> + SymbolInfo::SignalMap NewSymbols) override {
>> +for (const auto  : NewSymbols)
>> 
>> A new catch: `NewSymbols` should be passed by reference, otherwise a copy
>> will be generated.
>>
>> I did actually intend by-value here, FindAllSymbols no longer needs the
>> map once it's reported, so it is moved rather than copied (see the end of
>> FindAllSymbols.cpp)
>>
>> If this is too surprising, I can change it.
>>
>
> I'd say that that couples the call site and the function too much.
>
>
>>
>>
>>
>>
>>
>> https://reviews.llvm.org/D30210
>>
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Manuel Klimek via cfe-commits]

On Thu, Feb 23, 2017 at 10:40 PM Sam McCall  wrote:

>
>
> On Feb 23, 2017 8:48 PM, "Haojian Wu via Phabricator" <
> revi...@reviews.llvm.org> wrote:
>
> hokein added inline comments.
>
>
> 
> Comment at:
> unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp:40
> +  void reportSymbols(llvm::StringRef FileName,
> + SymbolInfo::SignalMap NewSymbols) override {
> +for (const auto  : NewSymbols)
> 
> A new catch: `NewSymbols` should be passed by reference, otherwise a copy
> will be generated.
>
> I did actually intend by-value here, FindAllSymbols no longer needs the
> map once it's reported, so it is moved rather than copied (see the end of
> FindAllSymbols.cpp)
>
> If this is too surprising, I can change it.
>

I'd say that that couples the call site and the function too much.


>
>
>
>
>
> https://reviews.llvm.org/D30210
>
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via cfe-commits]

On Feb 23, 2017 8:48 PM, "Haojian Wu via Phabricator" <
revi...@reviews.llvm.org> wrote:

hokein added inline comments.



Comment at: unittests/include-fixer/find-all-symbols/
FindAllSymbolsTests.cpp:40
+  void reportSymbols(llvm::StringRef FileName,
+ SymbolInfo::SignalMap NewSymbols) override {
+for (const auto  : NewSymbols)

A new catch: `NewSymbols` should be passed by reference, otherwise a copy
will be generated.

I did actually intend by-value here, FindAllSymbols no longer needs the map
once it's reported, so it is moved rather than copied (see the end of
FindAllSymbols.cpp)

If this is too surprising, I can change it.





https://reviews.llvm.org/D30210
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp:40
+  void reportSymbols(llvm::StringRef FileName,
+ SymbolInfo::SignalMap NewSymbols) override {
+for (const auto  : NewSymbols)

A new catch: `NewSymbols` should be passed by reference, otherwise a copy will 
be generated. 


https://reviews.llvm.org/D30210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 89510.
sammccall added a comment.

Update comment - oops!


https://reviews.llvm.org/D30210

Files:
  include-fixer/InMemorySymbolIndex.cpp
  include-fixer/InMemorySymbolIndex.h
  include-fixer/IncludeFixer.cpp
  include-fixer/SymbolIndex.h
  include-fixer/SymbolIndexManager.cpp
  include-fixer/YamlSymbolIndex.cpp
  include-fixer/YamlSymbolIndex.h
  include-fixer/find-all-symbols/FindAllMacros.cpp
  include-fixer/find-all-symbols/FindAllMacros.h
  include-fixer/find-all-symbols/FindAllSymbols.cpp
  include-fixer/find-all-symbols/FindAllSymbols.h
  include-fixer/find-all-symbols/FindAllSymbolsAction.cpp
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/find-all-symbols/SymbolReporter.h
  include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp
  include-fixer/tool/ClangIncludeFixer.cpp
  test/include-fixer/Inputs/fake_yaml_db.yaml
  test/include-fixer/Inputs/merge/a.yaml
  test/include-fixer/Inputs/merge/b.yaml
  test/include-fixer/merge.test
  unittests/include-fixer/IncludeFixerTest.cpp
  unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

Index: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
===
--- unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
+++ unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
@@ -19,6 +19,7 @@
 #include "clang/Frontend/PCHContainerOperations.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 "gtest/gtest.h"
@@ -31,34 +32,39 @@
 
 static const char HeaderName[] = "symbols.h";
 
-class TestSymbolReporter : public clang::find_all_symbols::SymbolReporter {
+class TestSymbolReporter : public SymbolReporter {
 public:
   ~TestSymbolReporter() override {}
 
-  void reportSymbol(llvm::StringRef FileName,
-const SymbolInfo ) override {
-Symbols.push_back(Symbol);
+  void reportSymbols(llvm::StringRef FileName,
+ SymbolInfo::SignalMap NewSymbols) override {
+for (const auto  : NewSymbols)
+  Symbols[Entry.first] += Entry.second;
   }
 
   bool hasSymbol(const SymbolInfo ) const {
-for (const auto  : Symbols) {
-  if (S == Symbol)
-return true;
-}
-return false;
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Seen > 0;
+  }
+
+  bool hasUse(const SymbolInfo ) const {
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Used > 0;
   }
 
 private:
-  std::vector Symbols;
+  SymbolInfo::SignalMap Symbols;
 };
 
 class FindAllSymbolsTest : public ::testing::Test {
 public:
   bool hasSymbol(const SymbolInfo ) {
 return Reporter.hasSymbol(Symbol);
   }
 
-  bool runFindAllSymbols(StringRef Code) {
+  bool hasUse(const SymbolInfo ) { return Reporter.hasUse(Symbol); }
+
+  bool runFindAllSymbols(StringRef HeaderCode, StringRef MainCode) {
 llvm::IntrusiveRefCntPtr InMemoryFileSystem(
 new vfs::InMemoryFileSystem);
 llvm::IntrusiveRefCntPtr Files(
@@ -88,7 +94,7 @@
 InMemoryFileSystem->addFile(InternalHeader, 0,
 llvm::MemoryBuffer::getMemBuffer(InternalCode));
 
-std::unique_ptr Factory(
+std::unique_ptr Factory(
 new FindAllSymbolsActionFactory(, ));
 
 tooling::ToolInvocation Invocation(
@@ -98,7 +104,7 @@
 std::make_shared());
 
 InMemoryFileSystem->addFile(HeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(Code));
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
 std::string Content = "#include\"" + std::string(HeaderName) +
   "\"\n"
@@ -118,6 +124,7 @@
 SymbolInfo DirtySymbol("ExtraInternal", SymbolInfo::SymbolKind::Class,
CleanHeader, 2, {});
 #endif // _MSC_VER && __MINGW32__
+Content += "\n" + MainCode.str();
 InMemoryFileSystem->addFile(FileName, 0,
 llvm::MemoryBuffer::getMemBuffer(Content));
 Invocation.run();
@@ -135,49 +142,64 @@
 };
 
 TEST_F(FindAllSymbolsTest, VariableSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern int xargc;
   namespace na {
   static bool  = false;
   namespace nb { const long long *; }
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  auto y = ::nb::;
+  int main() { if (na::) return xargc; }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("xargc", SymbolInfo::SymbolKind::Variable, HeaderName, 2, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 4,
   

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

Thanks! Looks good from my side.

I'd wait to see whether @bkramer has more comments before commit it.




Comment at: include-fixer/find-all-symbols/SymbolInfo.h:50
+  // used. These are used to rank results.
+  struct Signals {
+Signals() {}

hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > I think we can make a standalone class instead of making it a nested 
> > > class of `SymbolInfo` because I don't see strong relationship between 
> > > them. Maybe name it `FindingSignals` or `FindingInfo`.
> > The relationship between them is a strong scoping one: signals only make 
> > sense in the context of a particular SymbolInfo.
> > 
> > If it was a parallel top-level class, it needs a name that communicates 
> > this relationship, most likely SymbolSignals. I don't think that's 
> > significantly better than SymbolInfo::Signals.
> > 
> > (If I had my druthers, these would probably be Symbol and Symbol::Signals - 
> > the "info" is the main reason that SymbolInfo::Signals is noisy. But not 
> > worth the churn I think)
> > 
> You convinced me.  Please keep the comment of `Signals` updated.
> 
> > If I had my druthers, these would probably be Symbol and Symbol::Signals - 
> > the "info" is the main reason that SymbolInfo::Signals is noisy. But not 
> > worth the churn I think)
> 
> In the initial design, `SymbolInfo` merely represents a cpp symbol. But as 
> more features developed, `SymbolInfo` might be not a good name at the moment. 
> `Symbol` seems not a better name as LLVM already has many classes named 
> `Symbol`. We can leave it here.
> 
> 
>  keep the comment of Signals updated.

You are missing this.


https://reviews.llvm.org/D30210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for bearing with me here :)




Comment at: include-fixer/InMemorySymbolIndex.h:27
 
-  std::vector
+  std::vector
   search(llvm::StringRef Identifier) override;

hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > There are many places using 
> > > `std::vector`. Maybe we can 
> > > use a type alias for it, so that we can type less.  
> > I guess? It's the namespaces that are the problem (vector 
> > is fine) and most of the namespace noise wouldn't go away here.
> > 
> > is `clang::find_all_symbols::SymbolsSignalsList` better enough to obscure 
> > what the actual type is? It's 45 chars vs 54.
> > 
> > IMO it's not worth it here, though 
> > `clang::find_all_symbols::SymbolInfo::SignalMap` vs 
> > `std::map > clang::find_all_symbols::SymbolInfo::Signals>` is.
> If we put the type alias under `clang::include_fixer` namespace, it will 
> shorten the name more. Agree it is not worth the effect as the full name only 
> happens in headers. 
> 
> We could save a few characters by getting rid of `clang` because we are 
> always in `clang` namespace. So 
> `std::vector` should work, this looks 
> slightly better. :)
Done, also cleaned up other redundant namespaces in touched files.


https://reviews.llvm.org/D30210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 89506.
sammccall marked 3 inline comments as done.
sammccall added a comment.

Address review comments; remove redundant namespace qualifiers; format.


https://reviews.llvm.org/D30210

Files:
  include-fixer/InMemorySymbolIndex.cpp
  include-fixer/InMemorySymbolIndex.h
  include-fixer/IncludeFixer.cpp
  include-fixer/SymbolIndex.h
  include-fixer/SymbolIndexManager.cpp
  include-fixer/YamlSymbolIndex.cpp
  include-fixer/YamlSymbolIndex.h
  include-fixer/find-all-symbols/FindAllMacros.cpp
  include-fixer/find-all-symbols/FindAllMacros.h
  include-fixer/find-all-symbols/FindAllSymbols.cpp
  include-fixer/find-all-symbols/FindAllSymbols.h
  include-fixer/find-all-symbols/FindAllSymbolsAction.cpp
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/find-all-symbols/SymbolReporter.h
  include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp
  include-fixer/tool/ClangIncludeFixer.cpp
  test/include-fixer/Inputs/fake_yaml_db.yaml
  test/include-fixer/Inputs/merge/a.yaml
  test/include-fixer/Inputs/merge/b.yaml
  test/include-fixer/merge.test
  unittests/include-fixer/IncludeFixerTest.cpp
  unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

Index: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
===
--- unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
+++ unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
@@ -19,6 +19,7 @@
 #include "clang/Frontend/PCHContainerOperations.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 "gtest/gtest.h"
@@ -31,34 +32,39 @@
 
 static const char HeaderName[] = "symbols.h";
 
-class TestSymbolReporter : public clang::find_all_symbols::SymbolReporter {
+class TestSymbolReporter : public SymbolReporter {
 public:
   ~TestSymbolReporter() override {}
 
-  void reportSymbol(llvm::StringRef FileName,
-const SymbolInfo ) override {
-Symbols.push_back(Symbol);
+  void reportSymbols(llvm::StringRef FileName,
+ SymbolInfo::SignalMap NewSymbols) override {
+for (const auto  : NewSymbols)
+  Symbols[Entry.first] += Entry.second;
   }
 
   bool hasSymbol(const SymbolInfo ) const {
-for (const auto  : Symbols) {
-  if (S == Symbol)
-return true;
-}
-return false;
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Seen > 0;
+  }
+
+  bool hasUse(const SymbolInfo ) const {
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Used > 0;
   }
 
 private:
-  std::vector Symbols;
+  SymbolInfo::SignalMap Symbols;
 };
 
 class FindAllSymbolsTest : public ::testing::Test {
 public:
   bool hasSymbol(const SymbolInfo ) {
 return Reporter.hasSymbol(Symbol);
   }
 
-  bool runFindAllSymbols(StringRef Code) {
+  bool hasUse(const SymbolInfo ) { return Reporter.hasUse(Symbol); }
+
+  bool runFindAllSymbols(StringRef HeaderCode, StringRef MainCode) {
 llvm::IntrusiveRefCntPtr InMemoryFileSystem(
 new vfs::InMemoryFileSystem);
 llvm::IntrusiveRefCntPtr Files(
@@ -88,7 +94,7 @@
 InMemoryFileSystem->addFile(InternalHeader, 0,
 llvm::MemoryBuffer::getMemBuffer(InternalCode));
 
-std::unique_ptr Factory(
+std::unique_ptr Factory(
 new FindAllSymbolsActionFactory(, ));
 
 tooling::ToolInvocation Invocation(
@@ -98,7 +104,7 @@
 std::make_shared());
 
 InMemoryFileSystem->addFile(HeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(Code));
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
 std::string Content = "#include\"" + std::string(HeaderName) +
   "\"\n"
@@ -118,6 +124,7 @@
 SymbolInfo DirtySymbol("ExtraInternal", SymbolInfo::SymbolKind::Class,
CleanHeader, 2, {});
 #endif // _MSC_VER && __MINGW32__
+Content += "\n" + MainCode.str();
 InMemoryFileSystem->addFile(FileName, 0,
 llvm::MemoryBuffer::getMemBuffer(Content));
 Invocation.run();
@@ -135,49 +142,64 @@
 };
 
 TEST_F(FindAllSymbolsTest, VariableSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern int xargc;
   namespace na {
   static bool  = false;
   namespace nb { const long long *; }
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  auto y = ::nb::;
+  int main() { if (na::) return xargc; }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("xargc", SymbolInfo::SymbolKind::Variable, HeaderName, 2, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: include-fixer/InMemorySymbolIndex.h:27
 
-  std::vector
+  std::vector
   search(llvm::StringRef Identifier) override;

sammccall wrote:
> hokein wrote:
> > There are many places using 
> > `std::vector`. Maybe we can use 
> > a type alias for it, so that we can type less.  
> I guess? It's the namespaces that are the problem (vector 
> is fine) and most of the namespace noise wouldn't go away here.
> 
> is `clang::find_all_symbols::SymbolsSignalsList` better enough to obscure 
> what the actual type is? It's 45 chars vs 54.
> 
> IMO it's not worth it here, though 
> `clang::find_all_symbols::SymbolInfo::SignalMap` vs 
> `std::map clang::find_all_symbols::SymbolInfo::Signals>` is.
If we put the type alias under `clang::include_fixer` namespace, it will 
shorten the name more. Agree it is not worth the effect as the full name only 
happens in headers. 

We could save a few characters by getting rid of `clang` because we are always 
in `clang` namespace. So `std::vector` 
should work, this looks slightly better. :)



Comment at: include-fixer/find-all-symbols/FindAllMacros.cpp:38
+  if (auto Symbol = CreateMacroSymbol(MacroNameTok, MD->getMacroInfo()))
+FileSymbols[*Symbol].Seen++;
+}

code style: use prefix `++`. The same below.



Comment at: include-fixer/find-all-symbols/FindAllMacros.h:39
+
+  void Ifdef(SourceLocation Loc, const Token ,
+ const MacroDefinition ) override;

sammccall wrote:
> hokein wrote:
> > We are missing tests for these macro usages.
> These are covered by FindAllSymbolsTests, which (despite the name) tests the 
> whole FindAllSymbolsAction.
> 
> Specifically, MacroTest and MacroTestWithIWYU cover these.
Acked. You combined them in current tests (I originally thought there should be 
some separate tests for these).



Comment at: include-fixer/find-all-symbols/SymbolInfo.cpp:79
const std::vector ,
-   unsigned NumOccurrences)
+   unsigned NumOccurrences, unsigned NumUses)
 : Name(Name), Type(Type), FilePath(FilePath), Contexts(Contexts),

You forgot to update this?



Comment at: include-fixer/find-all-symbols/SymbolInfo.h:50
+  // used. These are used to rank results.
+  struct Signals {
+Signals() {}

sammccall wrote:
> hokein wrote:
> > I think we can make a standalone class instead of making it a nested class 
> > of `SymbolInfo` because I don't see strong relationship between them. Maybe 
> > name it `FindingSignals` or `FindingInfo`.
> The relationship between them is a strong scoping one: signals only make 
> sense in the context of a particular SymbolInfo.
> 
> If it was a parallel top-level class, it needs a name that communicates this 
> relationship, most likely SymbolSignals. I don't think that's significantly 
> better than SymbolInfo::Signals.
> 
> (If I had my druthers, these would probably be Symbol and Symbol::Signals - 
> the "info" is the main reason that SymbolInfo::Signals is noisy. But not 
> worth the churn I think)
> 
You convinced me.  Please keep the comment of `Signals` updated.

> If I had my druthers, these would probably be Symbol and Symbol::Signals - 
> the "info" is the main reason that SymbolInfo::Signals is noisy. But not 
> worth the churn I think)

In the initial design, `SymbolInfo` merely represents a cpp symbol. But as more 
features developed, `SymbolInfo` might be not a good name at the moment. 
`Symbol` seems not a better name as LLVM already has many classes named 
`Symbol`. We can leave it here.





Comment at: include-fixer/find-all-symbols/SymbolInfo.h:101
 private:
-  friend struct llvm::yaml::MappingTraits;
+  friend struct llvm::yaml::MappingTraits;
 

sammccall wrote:
> hokein wrote:
> > I'd put this statement inside `SymbolAndSignals`.
> That won't compile: it's the members of SymbolInfo that are private, not the 
> members of SymbolAndSignals.
Acked. Thanks for explanations. Sorry for misleading.



Comment at: include-fixer/find-all-symbols/SymbolInfo.h:129
 
-  /// \brief The number of times this symbol was found during an indexing
-  /// run. Populated by the reducer and used to rank results.
-  unsigned NumOccurrences;
+struct SymbolAndSignals {
+  SymbolInfo Symbol;

sammccall wrote:
> hokein wrote:
> > Not much better idea on names, how about `SymbolFinding`?
> > 
> > ```
> > struct SymbolFinding {
> >SymbolInfo Symbol;
> >FindingInfo Finding;
> > };
> > ```
> I don't think SymbolFinding is better:
>  - it can be misinterpreted as finding *for* a signal, not findings *and* a 
> signal. I think the And is important
>  - "finding" is vague while "signal" is more specific. I changed this from 
> finding -> signal already based 

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: include-fixer/InMemorySymbolIndex.h:27
 
-  std::vector
+  std::vector
   search(llvm::StringRef Identifier) override;

hokein wrote:
> There are many places using 
> `std::vector`. Maybe we can use a 
> type alias for it, so that we can type less.  
I guess? It's the namespaces that are the problem (vector is 
fine) and most of the namespace noise wouldn't go away here.

is `clang::find_all_symbols::SymbolsSignalsList` better enough to obscure what 
the actual type is? It's 45 chars vs 54.

IMO it's not worth it here, though 
`clang::find_all_symbols::SymbolInfo::SignalMap` vs 
`std::map` is.



Comment at: include-fixer/find-all-symbols/FindAllMacros.h:39
+
+  void Ifdef(SourceLocation Loc, const Token ,
+ const MacroDefinition ) override;

hokein wrote:
> We are missing tests for these macro usages.
These are covered by FindAllSymbolsTests, which (despite the name) tests the 
whole FindAllSymbolsAction.

Specifically, MacroTest and MacroTestWithIWYU cover these.



Comment at: include-fixer/find-all-symbols/SymbolInfo.h:50
+  // used. These are used to rank results.
+  struct Signals {
+Signals() {}

hokein wrote:
> I think we can make a standalone class instead of making it a nested class of 
> `SymbolInfo` because I don't see strong relationship between them. Maybe name 
> it `FindingSignals` or `FindingInfo`.
The relationship between them is a strong scoping one: signals only make sense 
in the context of a particular SymbolInfo.

If it was a parallel top-level class, it needs a name that communicates this 
relationship, most likely SymbolSignals. I don't think that's significantly 
better than SymbolInfo::Signals.

(If I had my druthers, these would probably be Symbol and Symbol::Signals - the 
"info" is the main reason that SymbolInfo::Signals is noisy. But not worth the 
churn I think)




Comment at: include-fixer/find-all-symbols/SymbolInfo.h:101
 private:
-  friend struct llvm::yaml::MappingTraits;
+  friend struct llvm::yaml::MappingTraits;
 

hokein wrote:
> I'd put this statement inside `SymbolAndSignals`.
That won't compile: it's the members of SymbolInfo that are private, not the 
members of SymbolAndSignals.



Comment at: include-fixer/find-all-symbols/SymbolInfo.h:129
 
-  /// \brief The number of times this symbol was found during an indexing
-  /// run. Populated by the reducer and used to rank results.
-  unsigned NumOccurrences;
+struct SymbolAndSignals {
+  SymbolInfo Symbol;

hokein wrote:
> Not much better idea on names, how about `SymbolFinding`?
> 
> ```
> struct SymbolFinding {
>SymbolInfo Symbol;
>FindingInfo Finding;
> };
> ```
I don't think SymbolFinding is better:
 - it can be misinterpreted as finding *for* a signal, not findings *and* a 
signal. I think the And is important
 - "finding" is vague while "signal" is more specific. I changed this from 
finding -> signal already based on a discussion with Ben, if you do want to 
change this we should sync up offline :)


https://reviews.llvm.org/D30210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 89484.
sammccall marked 4 inline comments as done.
sammccall added a comment.

Address review comments.


https://reviews.llvm.org/D30210

Files:
  include-fixer/InMemorySymbolIndex.cpp
  include-fixer/InMemorySymbolIndex.h
  include-fixer/IncludeFixer.cpp
  include-fixer/SymbolIndex.h
  include-fixer/SymbolIndexManager.cpp
  include-fixer/YamlSymbolIndex.cpp
  include-fixer/YamlSymbolIndex.h
  include-fixer/find-all-symbols/FindAllMacros.cpp
  include-fixer/find-all-symbols/FindAllMacros.h
  include-fixer/find-all-symbols/FindAllSymbols.cpp
  include-fixer/find-all-symbols/FindAllSymbols.h
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/find-all-symbols/SymbolReporter.h
  include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp
  include-fixer/tool/ClangIncludeFixer.cpp
  test/include-fixer/Inputs/fake_yaml_db.yaml
  test/include-fixer/Inputs/merge/a.yaml
  test/include-fixer/Inputs/merge/b.yaml
  test/include-fixer/merge.test
  unittests/include-fixer/IncludeFixerTest.cpp
  unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

Index: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
===
--- unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
+++ unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
@@ -19,6 +19,7 @@
 #include "clang/Frontend/PCHContainerOperations.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 "gtest/gtest.h"
@@ -35,30 +36,35 @@
 public:
   ~TestSymbolReporter() override {}
 
-  void reportSymbol(llvm::StringRef FileName,
-const SymbolInfo ) override {
-Symbols.push_back(Symbol);
+  void reportSymbols(llvm::StringRef FileName,
+ SymbolInfo::SignalMap NewSymbols) override {
+for (const auto  : NewSymbols)
+  Symbols[Entry.first] += Entry.second;
   }
 
   bool hasSymbol(const SymbolInfo ) const {
-for (const auto  : Symbols) {
-  if (S == Symbol)
-return true;
-}
-return false;
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Seen > 0;
+  }
+
+  bool hasUse(const SymbolInfo ) const {
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Used > 0;
   }
 
 private:
-  std::vector Symbols;
+  SymbolInfo::SignalMap Symbols;
 };
 
 class FindAllSymbolsTest : public ::testing::Test {
 public:
   bool hasSymbol(const SymbolInfo ) {
 return Reporter.hasSymbol(Symbol);
   }
 
-  bool runFindAllSymbols(StringRef Code) {
+  bool hasUse(const SymbolInfo ) { return Reporter.hasUse(Symbol); }
+
+  bool runFindAllSymbols(StringRef HeaderCode, StringRef MainCode) {
 llvm::IntrusiveRefCntPtr InMemoryFileSystem(
 new vfs::InMemoryFileSystem);
 llvm::IntrusiveRefCntPtr Files(
@@ -98,7 +104,7 @@
 std::make_shared());
 
 InMemoryFileSystem->addFile(HeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(Code));
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
 std::string Content = "#include\"" + std::string(HeaderName) +
   "\"\n"
@@ -118,6 +124,7 @@
 SymbolInfo DirtySymbol("ExtraInternal", SymbolInfo::SymbolKind::Class,
CleanHeader, 2, {});
 #endif // _MSC_VER && __MINGW32__
+Content += "\n" + MainCode.str();
 InMemoryFileSystem->addFile(FileName, 0,
 llvm::MemoryBuffer::getMemBuffer(Content));
 Invocation.run();
@@ -135,49 +142,64 @@
 };
 
 TEST_F(FindAllSymbolsTest, VariableSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern int xargc;
   namespace na {
   static bool  = false;
   namespace nb { const long long *; }
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  auto y = ::nb::;
+  int main() { if (na::) return xargc; }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("xargc", SymbolInfo::SymbolKind::Variable, HeaderName, 2, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 4,
   {{SymbolInfo::ContextType::Namespace, "na"}});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 5,
   {{SymbolInfo::ContextType::Namespace, "nb"},
{SymbolInfo::ContextType::Namespace, "na"}});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 }
 
 TEST_F(FindAllSymbolsTest, ExternCSymbols) {
-  static const char Code[] = 

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: include-fixer/InMemorySymbolIndex.h:27
 
-  std::vector
+  std::vector
   search(llvm::StringRef Identifier) override;

There are many places using 
`std::vector`. Maybe we can use a 
type alias for it, so that we can type less.  



Comment at: include-fixer/SymbolIndexManager.cpp:104
+for (const auto  : Symbols) {
+  const SymbolInfo Symbol = SymAndSig.Symbol;
   // Match the identifier name without qualifier.

I think you miss `&` here.



Comment at: include-fixer/find-all-symbols/FindAllMacros.cpp:37
  const MacroDirective *MD) {
-  SourceLocation Loc = SM->getExpansionLoc(MacroNameTok.getLocation());
-  std::string FilePath = getIncludePath(*SM, Loc, Collector);
-  if (FilePath.empty()) return;
+  if (auto Symbol = CreateMacroSymbol(MacroNameTok, MD->getMacroInfo())) {
+FileSymbols[*Symbol].Seen++;

Nit: No `{}` for single statement in `if`. The same below.



Comment at: include-fixer/find-all-symbols/FindAllMacros.h:39
+
+  void Ifdef(SourceLocation Loc, const Token ,
+ const MacroDefinition ) override;

We are missing tests for these macro usages.



Comment at: include-fixer/find-all-symbols/FindAllSymbols.h:52
+  std::string Filename;
+  // Findings for the current source file, flushed on EndSourceFileAction.
+  SymbolInfo::SignalMap FileSymbols;

s/`on EndSourceFileAction`/`onEndOfTranslationUnit`.



Comment at: include-fixer/find-all-symbols/SymbolInfo.h:50
+  // used. These are used to rank results.
+  struct Signals {
+Signals() {}

I think we can make a standalone class instead of making it a nested class of 
`SymbolInfo` because I don't see strong relationship between them. Maybe name 
it `FindingSignals` or `FindingInfo`.



Comment at: include-fixer/find-all-symbols/SymbolInfo.h:72
  int LineNumber, const std::vector ,
- unsigned NumOccurrences = 0);
+ unsigned NumOccurrences = 0, unsigned NumUses = 0);
 

We don't need this method since we have `SymbolAndSignals`?



Comment at: include-fixer/find-all-symbols/SymbolInfo.h:101
 private:
-  friend struct llvm::yaml::MappingTraits;
+  friend struct llvm::yaml::MappingTraits;
 

I'd put this statement inside `SymbolAndSignals`.



Comment at: include-fixer/find-all-symbols/SymbolInfo.h:129
 
-  /// \brief The number of times this symbol was found during an indexing
-  /// run. Populated by the reducer and used to rank results.
-  unsigned NumOccurrences;
+struct SymbolAndSignals {
+  SymbolInfo Symbol;

Not much better idea on names, how about `SymbolFinding`?

```
struct SymbolFinding {
   SymbolInfo Symbol;
   FindingInfo Finding;
};
```


https://reviews.llvm.org/D30210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 89366.
sammccall added a comment.

git-clang-format


https://reviews.llvm.org/D30210

Files:
  include-fixer/InMemorySymbolIndex.cpp
  include-fixer/InMemorySymbolIndex.h
  include-fixer/IncludeFixer.cpp
  include-fixer/SymbolIndex.h
  include-fixer/SymbolIndexManager.cpp
  include-fixer/YamlSymbolIndex.cpp
  include-fixer/YamlSymbolIndex.h
  include-fixer/find-all-symbols/FindAllMacros.cpp
  include-fixer/find-all-symbols/FindAllMacros.h
  include-fixer/find-all-symbols/FindAllSymbols.cpp
  include-fixer/find-all-symbols/FindAllSymbols.h
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/find-all-symbols/SymbolReporter.h
  include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp
  include-fixer/tool/ClangIncludeFixer.cpp
  test/include-fixer/Inputs/fake_yaml_db.yaml
  test/include-fixer/Inputs/merge/a.yaml
  test/include-fixer/Inputs/merge/b.yaml
  test/include-fixer/merge.test
  unittests/include-fixer/IncludeFixerTest.cpp
  unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

Index: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
===
--- unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
+++ unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
@@ -19,6 +19,7 @@
 #include "clang/Frontend/PCHContainerOperations.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 "gtest/gtest.h"
@@ -35,30 +36,35 @@
 public:
   ~TestSymbolReporter() override {}
 
-  void reportSymbol(llvm::StringRef FileName,
-const SymbolInfo ) override {
-Symbols.push_back(Symbol);
+  void reportSymbols(llvm::StringRef FileName,
+ SymbolInfo::SignalMap NewSymbols) override {
+for (const auto  : NewSymbols)
+  Symbols[Entry.first] += Entry.second;
   }
 
   bool hasSymbol(const SymbolInfo ) const {
-for (const auto  : Symbols) {
-  if (S == Symbol)
-return true;
-}
-return false;
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Seen > 0;
+  }
+
+  bool hasUse(const SymbolInfo ) const {
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Used > 0;
   }
 
 private:
-  std::vector Symbols;
+  SymbolInfo::SignalMap Symbols;
 };
 
 class FindAllSymbolsTest : public ::testing::Test {
 public:
   bool hasSymbol(const SymbolInfo ) {
 return Reporter.hasSymbol(Symbol);
   }
 
-  bool runFindAllSymbols(StringRef Code) {
+  bool hasUse(const SymbolInfo ) { return Reporter.hasUse(Symbol); }
+
+  bool runFindAllSymbols(StringRef HeaderCode, StringRef MainCode) {
 llvm::IntrusiveRefCntPtr InMemoryFileSystem(
 new vfs::InMemoryFileSystem);
 llvm::IntrusiveRefCntPtr Files(
@@ -98,7 +104,7 @@
 std::make_shared());
 
 InMemoryFileSystem->addFile(HeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(Code));
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
 std::string Content = "#include\"" + std::string(HeaderName) +
   "\"\n"
@@ -118,6 +124,7 @@
 SymbolInfo DirtySymbol("ExtraInternal", SymbolInfo::SymbolKind::Class,
CleanHeader, 2, {});
 #endif // _MSC_VER && __MINGW32__
+Content += "\n" + MainCode.str();
 InMemoryFileSystem->addFile(FileName, 0,
 llvm::MemoryBuffer::getMemBuffer(Content));
 Invocation.run();
@@ -135,49 +142,64 @@
 };
 
 TEST_F(FindAllSymbolsTest, VariableSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern int xargc;
   namespace na {
   static bool  = false;
   namespace nb { const long long *; }
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  auto y = ::nb::;
+  int main() { if (na::) return xargc; }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("xargc", SymbolInfo::SymbolKind::Variable, HeaderName, 2, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 4,
   {{SymbolInfo::ContextType::Namespace, "na"}});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 5,
   {{SymbolInfo::ContextType::Namespace, "nb"},
{SymbolInfo::ContextType::Namespace, "na"}});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 }
 
 TEST_F(FindAllSymbolsTest, ExternCSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern 

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 89365.
sammccall added a comment.

Switch from SymbolInfo::WithSignals as a typedef for std::pair, to a struct
SymbolAndSignals, to have clearer semantics.


https://reviews.llvm.org/D30210

Files:
  include-fixer/InMemorySymbolIndex.cpp
  include-fixer/InMemorySymbolIndex.h
  include-fixer/IncludeFixer.cpp
  include-fixer/SymbolIndex.h
  include-fixer/SymbolIndexManager.cpp
  include-fixer/YamlSymbolIndex.cpp
  include-fixer/YamlSymbolIndex.h
  include-fixer/find-all-symbols/FindAllMacros.cpp
  include-fixer/find-all-symbols/FindAllMacros.h
  include-fixer/find-all-symbols/FindAllSymbols.cpp
  include-fixer/find-all-symbols/FindAllSymbols.h
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/find-all-symbols/SymbolReporter.h
  include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp
  include-fixer/tool/ClangIncludeFixer.cpp
  test/include-fixer/Inputs/fake_yaml_db.yaml
  test/include-fixer/Inputs/merge/a.yaml
  test/include-fixer/Inputs/merge/b.yaml
  test/include-fixer/merge.test
  unittests/include-fixer/IncludeFixerTest.cpp
  unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

Index: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
===
--- unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
+++ unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
@@ -19,6 +19,7 @@
 #include "clang/Frontend/PCHContainerOperations.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 "gtest/gtest.h"
@@ -35,30 +36,36 @@
 public:
   ~TestSymbolReporter() override {}
 
-  void reportSymbol(llvm::StringRef FileName,
-const SymbolInfo ) override {
-Symbols.push_back(Symbol);
+  void reportSymbols(llvm::StringRef FileName,
+ SymbolInfo::SignalMap NewSymbols) override {
+for (const auto  : NewSymbols) Symbols[Entry.first] += Entry.second;
   }
 
   bool hasSymbol(const SymbolInfo ) const {
-for (const auto  : Symbols) {
-  if (S == Symbol)
-return true;
-}
-return false;
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Seen > 0;
+  }
+
+  bool hasUse(const SymbolInfo ) const {
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Used > 0;
   }
 
 private:
-  std::vector Symbols;
+  SymbolInfo::SignalMap Symbols;
 };
 
 class FindAllSymbolsTest : public ::testing::Test {
 public:
   bool hasSymbol(const SymbolInfo ) {
 return Reporter.hasSymbol(Symbol);
   }
 
-  bool runFindAllSymbols(StringRef Code) {
+  bool hasUse(const SymbolInfo ) {
+return Reporter.hasUse(Symbol);
+  }
+
+  bool runFindAllSymbols(StringRef HeaderCode, StringRef MainCode) {
 llvm::IntrusiveRefCntPtr InMemoryFileSystem(
 new vfs::InMemoryFileSystem);
 llvm::IntrusiveRefCntPtr Files(
@@ -98,7 +105,7 @@
 std::make_shared());
 
 InMemoryFileSystem->addFile(HeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(Code));
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
 std::string Content = "#include\"" + std::string(HeaderName) +
   "\"\n"
@@ -118,6 +125,7 @@
 SymbolInfo DirtySymbol("ExtraInternal", SymbolInfo::SymbolKind::Class,
CleanHeader, 2, {});
 #endif // _MSC_VER && __MINGW32__
+Content += "\n" + MainCode.str();
 InMemoryFileSystem->addFile(FileName, 0,
 llvm::MemoryBuffer::getMemBuffer(Content));
 Invocation.run();
@@ -135,49 +143,64 @@
 };
 
 TEST_F(FindAllSymbolsTest, VariableSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern int xargc;
   namespace na {
   static bool  = false;
   namespace nb { const long long *; }
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  auto y = ::nb::;
+  int main() { if (na::) return xargc; }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("xargc", SymbolInfo::SymbolKind::Variable, HeaderName, 2, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 4,
   {{SymbolInfo::ContextType::Namespace, "na"}});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 5,
   {{SymbolInfo::ContextType::Namespace, "nb"},
{SymbolInfo::ContextType::Namespace, "na"}});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 }
 
 

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 89360.
sammccall added a comment.

Changes based on offline discussion:

- mutable SignalInfo data split into SignalInfo::Signals
- yaml serialized data is SignalInfo::WithSignals, which is currently a 
pair
- FindAllSymbols/FindAllMacros now report in batches, with symbols already 
deduplicated. This makes it easy to count each (file,symbol) only once.
- implemented macro usages because I had to touch that anyway


https://reviews.llvm.org/D30210

Files:
  include-fixer/InMemorySymbolIndex.cpp
  include-fixer/InMemorySymbolIndex.h
  include-fixer/IncludeFixer.cpp
  include-fixer/SymbolIndex.h
  include-fixer/SymbolIndexManager.cpp
  include-fixer/YamlSymbolIndex.cpp
  include-fixer/YamlSymbolIndex.h
  include-fixer/find-all-symbols/FindAllMacros.cpp
  include-fixer/find-all-symbols/FindAllMacros.h
  include-fixer/find-all-symbols/FindAllSymbols.cpp
  include-fixer/find-all-symbols/FindAllSymbols.h
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/find-all-symbols/SymbolReporter.h
  include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp
  include-fixer/tool/ClangIncludeFixer.cpp
  test/include-fixer/Inputs/fake_yaml_db.yaml
  test/include-fixer/Inputs/merge/a.yaml
  test/include-fixer/Inputs/merge/b.yaml
  test/include-fixer/merge.test
  unittests/include-fixer/IncludeFixerTest.cpp
  unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

Index: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
===
--- unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
+++ unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
@@ -19,6 +19,7 @@
 #include "clang/Frontend/PCHContainerOperations.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 "gtest/gtest.h"
@@ -35,30 +36,36 @@
 public:
   ~TestSymbolReporter() override {}
 
-  void reportSymbol(llvm::StringRef FileName,
-const SymbolInfo ) override {
-Symbols.push_back(Symbol);
+  void reportSymbols(llvm::StringRef FileName,
+ SymbolInfo::SignalMap NewSymbols) override {
+for (const auto  : NewSymbols) Symbols[Entry.first] += Entry.second;
   }
 
   bool hasSymbol(const SymbolInfo ) const {
-for (const auto  : Symbols) {
-  if (S == Symbol)
-return true;
-}
-return false;
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Seen > 0;
+  }
+
+  bool hasUse(const SymbolInfo ) const {
+auto it = Symbols.find(Symbol);
+return it != Symbols.end() && it->second.Used > 0;
   }
 
 private:
-  std::vector Symbols;
+  SymbolInfo::SignalMap Symbols;
 };
 
 class FindAllSymbolsTest : public ::testing::Test {
 public:
   bool hasSymbol(const SymbolInfo ) {
 return Reporter.hasSymbol(Symbol);
   }
 
-  bool runFindAllSymbols(StringRef Code) {
+  bool hasUse(const SymbolInfo ) {
+return Reporter.hasUse(Symbol);
+  }
+
+  bool runFindAllSymbols(StringRef HeaderCode, StringRef MainCode) {
 llvm::IntrusiveRefCntPtr InMemoryFileSystem(
 new vfs::InMemoryFileSystem);
 llvm::IntrusiveRefCntPtr Files(
@@ -98,7 +105,7 @@
 std::make_shared());
 
 InMemoryFileSystem->addFile(HeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(Code));
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
 std::string Content = "#include\"" + std::string(HeaderName) +
   "\"\n"
@@ -118,6 +125,7 @@
 SymbolInfo DirtySymbol("ExtraInternal", SymbolInfo::SymbolKind::Class,
CleanHeader, 2, {});
 #endif // _MSC_VER && __MINGW32__
+Content += "\n" + MainCode.str();
 InMemoryFileSystem->addFile(FileName, 0,
 llvm::MemoryBuffer::getMemBuffer(Content));
 Invocation.run();
@@ -135,49 +143,64 @@
 };
 
 TEST_F(FindAllSymbolsTest, VariableSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern int xargc;
   namespace na {
   static bool  = false;
   namespace nb { const long long *; }
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  auto y = ::nb::;
+  int main() { if (na::) return xargc; }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("xargc", SymbolInfo::SymbolKind::Variable, HeaderName, 2, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 4,
   {{SymbolInfo::ContextType::Namespace, "na"}});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", 

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall planned changes to this revision.
sammccall added a comment.

OK, some changes to come here:

- klimek thinks using a set as a pseudo-map with `mutable` is evil, which is a 
fair point
- the current `SymbolReporter` interface doesn't provide enough information to 
deduplicate efficiently, so we'll move the dedup to `FindAllSymbols` instead.




Comment at: include-fixer/find-all-symbols/SymbolInfo.h:79-80
 
-  /// \brief The number of times this symbol was found during an indexing run.
-  unsigned getNumOccurrences() const { return NumOccurrences; }
+  // Ranking signals are mutable to allow updating when the SymbolInfo is a map
+  // key. They do not affect ordering or equality.
+  /// \brief The number of times this symbol was found during an indexing

klimek wrote:
> So we map from Symbol to NumOccurences in SymbolInfoMain, but duplicate the 
> info in the key? That seems somewhat weird.
We no longer do that mapping.


https://reviews.llvm.org/D30210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.



Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:251
+  } else {
+assert(false && "Must match a NamedDecl!");
+  }

You can use llvm_unreachable  instead. Or do you actually want to fall through 
in release mode?



Comment at: include-fixer/find-all-symbols/SymbolInfo.h:79-80
 
-  /// \brief The number of times this symbol was found during an indexing run.
-  unsigned getNumOccurrences() const { return NumOccurrences; }
+  // Ranking signals are mutable to allow updating when the SymbolInfo is a map
+  // key. They do not affect ordering or equality.
+  /// \brief The number of times this symbol was found during an indexing

So we map from Symbol to NumOccurences in SymbolInfoMain, but duplicate the 
info in the key? That seems somewhat weird.


https://reviews.llvm.org/D30210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for the review, still learning the style :)




Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:250
+  } else {
+assert(!"Must match a NamedDecl!");
+  }

hokein wrote:
> Is the preceding `!` intended?
This does the right thing (!"string" is always false) but generates a warning.
Changed to assert(false && ...)


https://reviews.llvm.org/D30210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 89337.
sammccall marked 7 inline comments as done.
sammccall added a comment.

Address review comments.


https://reviews.llvm.org/D30210

Files:
  include-fixer/IncludeFixer.cpp
  include-fixer/SymbolIndexManager.cpp
  include-fixer/find-all-symbols/FindAllSymbols.cpp
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/find-all-symbols/SymbolReporter.h
  include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp
  test/include-fixer/Inputs/fake_yaml_db.yaml
  test/include-fixer/Inputs/merge/a.yaml
  test/include-fixer/Inputs/merge/b.yaml
  test/include-fixer/merge.test
  unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

Index: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
===
--- unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
+++ unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
@@ -19,6 +19,7 @@
 #include "clang/Frontend/PCHContainerOperations.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 "gtest/gtest.h"
@@ -40,25 +41,34 @@
 Symbols.push_back(Symbol);
   }
 
+  void reportUse(llvm::StringRef FileName, const SymbolInfo ) override {
+Used.push_back(Symbol);
+  }
+
   bool hasSymbol(const SymbolInfo ) const {
-for (const auto  : Symbols) {
-  if (S == Symbol)
-return true;
-}
-return false;
+return llvm::is_contained(Symbols, Symbol);
+  }
+
+  bool hasUse(const SymbolInfo ) const {
+return llvm::is_contained(Used, Symbol);
   }
 
 private:
   std::vector Symbols;
+  std::vector Used;
 };
 
 class FindAllSymbolsTest : public ::testing::Test {
 public:
   bool hasSymbol(const SymbolInfo ) {
 return Reporter.hasSymbol(Symbol);
   }
 
-  bool runFindAllSymbols(StringRef Code) {
+  bool hasUse(const SymbolInfo ) {
+return Reporter.hasUse(Symbol);
+  }
+
+  bool runFindAllSymbols(StringRef HeaderCode, StringRef MainCode) {
 llvm::IntrusiveRefCntPtr InMemoryFileSystem(
 new vfs::InMemoryFileSystem);
 llvm::IntrusiveRefCntPtr Files(
@@ -98,7 +108,7 @@
 std::make_shared());
 
 InMemoryFileSystem->addFile(HeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(Code));
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
 std::string Content = "#include\"" + std::string(HeaderName) +
   "\"\n"
@@ -118,6 +128,7 @@
 SymbolInfo DirtySymbol("ExtraInternal", SymbolInfo::SymbolKind::Class,
CleanHeader, 2, {});
 #endif // _MSC_VER && __MINGW32__
+Content += "\n" + MainCode.str();
 InMemoryFileSystem->addFile(FileName, 0,
 llvm::MemoryBuffer::getMemBuffer(Content));
 Invocation.run();
@@ -135,49 +146,64 @@
 };
 
 TEST_F(FindAllSymbolsTest, VariableSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern int xargc;
   namespace na {
   static bool  = false;
   namespace nb { const long long *; }
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  auto y = ::nb::;
+  int main() { if (na::) return xargc; }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("xargc", SymbolInfo::SymbolKind::Variable, HeaderName, 2, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 4,
   {{SymbolInfo::ContextType::Namespace, "na"}});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 5,
   {{SymbolInfo::ContextType::Namespace, "nb"},
{SymbolInfo::ContextType::Namespace, "na"}});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 }
 
 TEST_F(FindAllSymbolsTest, ExternCSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern "C" {
   int C_Func() { return 0; }
   struct C_struct {
 int Member;
   };
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  C_struct q() {
+int(*ptr)() = C_Func;
+return {0};
+  }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("C_Func", SymbolInfo::SymbolKind::Function, HeaderName, 3, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol =
   SymbolInfo("C_struct", SymbolInfo::SymbolKind::Class, HeaderName, 4, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 }
 
 TEST_F(FindAllSymbolsTest, CXXRecordSymbols) {
-  

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the contributions!




Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:244
 
-  const auto *ND = Result.Nodes.getNodeAs("decl");
-  assert(ND && "Matched declaration must be a NamedDecl!");
+  auto report = ::reportSymbol;
+  const NamedDecl *ND;

The way seems too tricky to me. I'd use a flag variable like `isUsedDecl` to 
distinguish and handle `use` and `decl` cases.



Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:250
+  } else {
+assert(!"Must match a NamedDecl!");
+  }

Is the preceding `!` intended?



Comment at: include-fixer/find-all-symbols/SymbolInfo.h:83
+  /// run. Populated by the reducer and used to rank results.
+  mutable unsigned NumOccurrences;
+  /// \brief The number of times this symbol was used during an indexing run.

A blank between class members.



Comment at: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp:551
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_FALSE(hasUse(Symbol));  // Not yet implemented.
 

The same.



Comment at: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp:48
   bool hasSymbol(const SymbolInfo ) const {
-for (const auto  : Symbols) {
-  if (S == Symbol)
-return true;
-}
-return false;
+return std::find(Symbols.begin(), Symbols.end(), Symbol) != Symbols.end();
+  }

nit: You can use `llvm::is_contained` here.



Comment at: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp:52
+  bool hasUse(const SymbolInfo ) const {
+return std::find(Used.begin(), Used.end(), Symbol) != Used.end();
   }

The same.



Comment at: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp:65
   }
+  bool hasUse(const SymbolInfo ) {
+return Reporter.hasUse(Symbol);

nit: a blank line between methods.



Comment at: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp:526
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_FALSE(hasUse(Symbol));  // Not yet implemented.
 

I'd put a `FIXME` comment here.


https://reviews.llvm.org/D30210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 89236.
sammccall added a comment.

Remove obsolete comment.


https://reviews.llvm.org/D30210

Files:
  include-fixer/IncludeFixer.cpp
  include-fixer/SymbolIndexManager.cpp
  include-fixer/find-all-symbols/FindAllSymbols.cpp
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/find-all-symbols/SymbolReporter.h
  include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp
  test/include-fixer/Inputs/fake_yaml_db.yaml
  test/include-fixer/Inputs/merge/a.yaml
  test/include-fixer/Inputs/merge/b.yaml
  test/include-fixer/merge.test
  unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

Index: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
===
--- unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
+++ unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
@@ -40,25 +40,33 @@
 Symbols.push_back(Symbol);
   }
 
+  void reportUse(llvm::StringRef FileName, const SymbolInfo ) override {
+Used.push_back(Symbol);
+  }
+
   bool hasSymbol(const SymbolInfo ) const {
-for (const auto  : Symbols) {
-  if (S == Symbol)
-return true;
-}
-return false;
+return std::find(Symbols.begin(), Symbols.end(), Symbol) != Symbols.end();
+  }
+
+  bool hasUse(const SymbolInfo ) const {
+return std::find(Used.begin(), Used.end(), Symbol) != Used.end();
   }
 
 private:
   std::vector Symbols;
+  std::vector Used;
 };
 
 class FindAllSymbolsTest : public ::testing::Test {
 public:
   bool hasSymbol(const SymbolInfo ) {
 return Reporter.hasSymbol(Symbol);
   }
+  bool hasUse(const SymbolInfo ) {
+return Reporter.hasUse(Symbol);
+  }
 
-  bool runFindAllSymbols(StringRef Code) {
+  bool runFindAllSymbols(StringRef HeaderCode, StringRef MainCode) {
 llvm::IntrusiveRefCntPtr InMemoryFileSystem(
 new vfs::InMemoryFileSystem);
 llvm::IntrusiveRefCntPtr Files(
@@ -98,7 +106,7 @@
 std::make_shared());
 
 InMemoryFileSystem->addFile(HeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(Code));
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
 std::string Content = "#include\"" + std::string(HeaderName) +
   "\"\n"
@@ -118,6 +126,7 @@
 SymbolInfo DirtySymbol("ExtraInternal", SymbolInfo::SymbolKind::Class,
CleanHeader, 2, {});
 #endif // _MSC_VER && __MINGW32__
+Content += "\n" + MainCode.str();
 InMemoryFileSystem->addFile(FileName, 0,
 llvm::MemoryBuffer::getMemBuffer(Content));
 Invocation.run();
@@ -135,49 +144,64 @@
 };
 
 TEST_F(FindAllSymbolsTest, VariableSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern int xargc;
   namespace na {
   static bool  = false;
   namespace nb { const long long *; }
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  auto y = ::nb::;
+  int main() { if (na::) return xargc; }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("xargc", SymbolInfo::SymbolKind::Variable, HeaderName, 2, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 4,
   {{SymbolInfo::ContextType::Namespace, "na"}});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 5,
   {{SymbolInfo::ContextType::Namespace, "nb"},
{SymbolInfo::ContextType::Namespace, "na"}});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 }
 
 TEST_F(FindAllSymbolsTest, ExternCSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern "C" {
   int C_Func() { return 0; }
   struct C_struct {
 int Member;
   };
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  C_struct q() {
+int(*ptr)() = C_Func;
+return {0};
+  }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("C_Func", SymbolInfo::SymbolKind::Function, HeaderName, 3, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol =
   SymbolInfo("C_struct", SymbolInfo::SymbolKind::Class, HeaderName, 4, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 }
 
 TEST_F(FindAllSymbolsTest, CXXRecordSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   struct Glob {};
   struct A; // Not a defintion, ignored.
   class NOP; // Not a defintion, ignored
@@ -190,26 +214,33 @@
   };
   };  //
   )";
-  runFindAllSymbols(Code);
+  

[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.

Add usage count to find-all-symbols.

FindAllSymbols now finds (most!) main-file usages of the discovered symbols.
The per-TU map output has NumUses=0 or 1 (only one use per file is counted).
The reducer aggregates these to find the number of files that use a symbol.

The NumOccurrences is now set to 1 in the mapper rather than being inferred by
the reducer, for consistency.

The idea here is to use NumUses for ranking: intuitively number of files that
use a symbol is more meaningful than number of files that include the header.


https://reviews.llvm.org/D30210

Files:
  include-fixer/IncludeFixer.cpp
  include-fixer/SymbolIndexManager.cpp
  include-fixer/find-all-symbols/FindAllSymbols.cpp
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/find-all-symbols/SymbolReporter.h
  include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp
  test/include-fixer/Inputs/fake_yaml_db.yaml
  test/include-fixer/Inputs/merge/a.yaml
  test/include-fixer/Inputs/merge/b.yaml
  test/include-fixer/merge.test
  unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

Index: unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
===
--- unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
+++ unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
@@ -40,25 +40,33 @@
 Symbols.push_back(Symbol);
   }
 
+  void reportUse(llvm::StringRef FileName, const SymbolInfo ) override {
+Used.push_back(Symbol);
+  }
+
   bool hasSymbol(const SymbolInfo ) const {
-for (const auto  : Symbols) {
-  if (S == Symbol)
-return true;
-}
-return false;
+return std::find(Symbols.begin(), Symbols.end(), Symbol) != Symbols.end();
+  }
+
+  bool hasUse(const SymbolInfo ) const {
+return std::find(Used.begin(), Used.end(), Symbol) != Used.end();
   }
 
 private:
   std::vector Symbols;
+  std::vector Used;
 };
 
 class FindAllSymbolsTest : public ::testing::Test {
 public:
   bool hasSymbol(const SymbolInfo ) {
 return Reporter.hasSymbol(Symbol);
   }
+  bool hasUse(const SymbolInfo ) {
+return Reporter.hasUse(Symbol);
+  }
 
-  bool runFindAllSymbols(StringRef Code) {
+  bool runFindAllSymbols(StringRef HeaderCode, StringRef MainCode) {
 llvm::IntrusiveRefCntPtr InMemoryFileSystem(
 new vfs::InMemoryFileSystem);
 llvm::IntrusiveRefCntPtr Files(
@@ -98,7 +106,7 @@
 std::make_shared());
 
 InMemoryFileSystem->addFile(HeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(Code));
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
 std::string Content = "#include\"" + std::string(HeaderName) +
   "\"\n"
@@ -118,6 +126,7 @@
 SymbolInfo DirtySymbol("ExtraInternal", SymbolInfo::SymbolKind::Class,
CleanHeader, 2, {});
 #endif // _MSC_VER && __MINGW32__
+Content += "\n" + MainCode.str();
 InMemoryFileSystem->addFile(FileName, 0,
 llvm::MemoryBuffer::getMemBuffer(Content));
 Invocation.run();
@@ -135,49 +144,64 @@
 };
 
 TEST_F(FindAllSymbolsTest, VariableSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern int xargc;
   namespace na {
   static bool  = false;
   namespace nb { const long long *; }
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  auto y = ::nb::;
+  int main() { if (na::) return xargc; }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("xargc", SymbolInfo::SymbolKind::Variable, HeaderName, 2, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 4,
   {{SymbolInfo::ContextType::Namespace, "na"}});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol = SymbolInfo("", SymbolInfo::SymbolKind::Variable, HeaderName, 5,
   {{SymbolInfo::ContextType::Namespace, "nb"},
{SymbolInfo::ContextType::Namespace, "na"}});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 }
 
 TEST_F(FindAllSymbolsTest, ExternCSymbols) {
-  static const char Code[] = R"(
+  static const char Header[] = R"(
   extern "C" {
   int C_Func() { return 0; }
   struct C_struct {
 int Member;
   };
   })";
-  runFindAllSymbols(Code);
+  static const char Main[] = R"(
+  C_struct q() {
+int(*ptr)() = C_Func;
+return {0};
+  }
+  )";
+  runFindAllSymbols(Header, Main);
 
   SymbolInfo Symbol =
   SymbolInfo("C_Func", SymbolInfo::SymbolKind::Function, HeaderName, 3, {});
   EXPECT_TRUE(hasSymbol(Symbol));
+  EXPECT_TRUE(hasUse(Symbol));
 
   Symbol =