[PATCH] D19482: [include-fixer] Add a find-all-symbols tool for include-fixer.

[Daniel Jasper via cfe-commits]

djasper added inline comments.

================
Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:22-24
@@ +21,5 @@
+
+using SymbolInfo = clang::find_all_symbols::SymbolInfo;
+using SymbolType = clang::find_all_symbols::SymbolInfo::SymbolType;
+using ContextType = clang::find_all_symbols::SymbolInfo::ContextType;
+
----------------
Can you remove the "abc = " part?

================
Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:29
@@ +28,3 @@
+namespace {
+void SetContext(const clang::NamedDecl *ND, SymbolInfo *Symbol) {
+  const auto *Context = ND->getDeclContext();
----------------
Remove "clang::". Here and everywhere.

================
Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:31
@@ +30,3 @@
+  const auto *Context = ND->getDeclContext();
+  while (Context) {
+    if (llvm::isa<clang::TranslationUnitDecl>(Context) ||
----------------
I'd turn this into a for loop:

  for (const auto *Context = ND->getDeclContext; Context; Context = Context->getParent()) {
    ...

================
Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:33
@@ +32,3 @@
+    if (llvm::isa<clang::TranslationUnitDecl>(Context) ||
+        llvm::isa<clang::LinkageSpecDecl>(Context)) {
+      return;
----------------
No braces. Here and at all the other single-statement ifs.

================
Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:37
@@ +36,3 @@
+
+    assert(llvm::isa<clang::NamedDecl>(Context) &&
+           "Expect Context to be a NamedDecl");
----------------
nit: remove "llvm::" and "clang::"

================
Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:40
@@ +39,3 @@
+    if (const auto *NSD = llvm::dyn_cast<clang::NamespaceDecl>(Context)) {
+      std::string Name;
+      if (!NSD->isAnonymousNamespace()) {
----------------
I'd just do:

  Symbol->Contexts.emplace_back(
      SymbolInfo::Namespace,
      NSD->isAnonymousNamespace() ? "" : NSD->getName().str());


================
Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:47
@@ +46,3 @@
+      const auto *RD = llvm::cast<clang::RecordDecl>(Context);
+      Symbol->Contexts.emplace_back(SymbolInfo::Record, RD->getName().str());
+    }
----------------
Maybe assert that RD isn't nullptr?

================
Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:76
@@ +75,3 @@
+
+void FindAllSymbols::registerSearcher(MatchFinder *MatchFinder) {
+  // FIXME: Handle specialization.
----------------
We traditionally call this registerMatchers ..

================
Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:201
@@ +200,3 @@
+  const clang::FileEntry *FE = SM->getFileEntryForID(SM->getMainFileID());
+  Reporter->reportResult(FE->getName(), Symbol);
+}
----------------
Why is it useful to report with the name of the main file? I think some more comments on what this tool is supposed to do overall would help. Maybe at the top of this file or of FindAllSymbolsMain.

================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:24
@@ +23,3 @@
+struct SymbolInfo {
+  enum SymbolType {
+    Function,
----------------
Call this "Kind" so that we don't confuse it with the Type of variables and function parameters.

================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:37
@@ +36,3 @@
+  /// \brief Identifier name.
+  std::string Identifier;
+
----------------
I'd just call this name.

================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:46
@@ +45,3 @@
+  /// \brief A pair of <ContextType, ContextName>.
+  typedef std::pair<ContextType, std::string> ContextInfo;
+
----------------
Maybe just call this Context instead of ContextInfo?

================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:52
@@ +51,3 @@
+  /// For example, if a symbol 'x' is declared as:
+  ///     namespace na { namespace nb { class A { int x; } } }
+  /// The contexts would be { {RECORD, "A"}, {NAMESPACE, "nb"}, {NAMESPACE,
----------------
Can't hurt to explicitly state that unnamed namespaces are stored with the name "".

================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:76
@@ +75,3 @@
+  /// \brief The function information.
+  llvm::Optional<FunctionInfo> FunctionInfos;
+
----------------
Should we maybe just use inheritance here?


http://reviews.llvm.org/D19482