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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
