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