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

[Haojian Wu via cfe-commits]

hokein 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;
+
----------------
djasper wrote:
> Can you remove the "abc = " part?
Remove all of these since they are not needed here.

================
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());
+    }
----------------
djasper wrote:
> Maybe assert that RD isn't nullptr?
`cast` does it for us.

================
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);
+}
----------------
djasper wrote:
> 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.
Yeah, the find-all-symbols tool can accept multiple files at one time ("find-all-symbols <source0>, source1, ..."), we need the name for the main file here. 

================
Comment at: include-fixer/find-all-symbols/SymbolInfo.h:76
@@ +75,3 @@
+  /// \brief The function information.
+  llvm::Optional<FunctionInfo> FunctionInfos;
+
----------------
djasper wrote:
> Should we maybe just use inheritance here?
We might not use inheritance here because LLVM yaml library doesn't support it.


http://reviews.llvm.org/D19482