ioeric added a comment.
The reduce logic seems to be in a good shape. Some nits and questions inlined.
================
Comment at: clang-doc/Reducer.cpp:19
+
+#define REDUCE(INFO)
\
+ {
\
----------------
It seems that you could use a template function?
================
Comment at: clang-doc/Reducer.cpp:24
+ for (auto &I : Values) {
\
+ if (!Tmp->merge(std::move(*static_cast<INFO *>(I.get()))))
\
+ return nullptr;
\
----------------
Do we really want to give up all infos when one bad info/merge is present?
================
Comment at: clang-doc/Representation.h:138
+ SymbolID USR =
+ SymbolID(); // Unique identifier for the decl described by this Info.
+ const InfoType IT = InfoType::IT_default; // InfoType of this particular
Info.
----------------
Shouldn't USR be `SymbolID()` by default?
================
Comment at: clang-doc/Representation.h:146
+protected:
+ bool mergeBase(Info &&I);
};
----------------
It's a bit awkward that users have to dispatch from info types to the
corresponding `merge` function (as in `Reducer.cpp`). I think it would make
users' life easier if the library handles the dispatching.
I wonder if something like the following would be better:
```
struct Info {
std::unique_ptr<Info> merge(const Indo& LHS, const Info& RHS);
};
// A standalone function.
std::unique_ptr<Info> mergeInfo(const Info &LHS, const Info& RHS) {
// merge base info.
...
// merge subclass infos.
assert(LHS.IT == RHS.IT); // or return nullptr
switch (LHS.IT) {
...
return Namespace::merge(LHS, RHS);
}
}
struct NamespaceInfo : public Info {
std::unique_ptr<Info> merge(LHS, RHS);
};
```
The unhandled switch case warning in compiler would help you catch
unimplemented `merge` when new info types are added.
================
Comment at: clang-doc/tool/ClangDocMain.cpp:60
+static llvm::cl::opt<bool>
+ DumpResult("dump",
----------------
If this only dumps the intermediate results, should we call it something like
`dump-intermediate` instead, to avoid confusion with final results?
================
Comment at: clang-doc/tool/ClangDocMain.cpp:148
+ });
+ return Err;
+ }
----------------
Print an error message on error?
================
Comment at: clang-doc/tool/ClangDocMain.cpp:154
+ llvm::StringMap<std::vector<std::unique_ptr<doc::Info>>> MapOutput;
+ Exec->get()->getToolResults()->forEachResult([&](StringRef Key,
+ StringRef Value) {
----------------
Could you add a brief comment explaining what key and value are?
================
Comment at: clang-doc/tool/ClangDocMain.cpp:161
+ for (auto &I : Infos) {
+ llvm::errs() << "COLLECT: " << llvm::toHex(llvm::toStringRef(I->USR))
+ << ": " << I->Name << "\n";
----------------
nit: remove debug logging?
================
Comment at: clang-doc/tool/ClangDocMain.cpp:170
+ // Reducing phase
+ llvm::outs() << "Reducing infos...\n";
+ for (auto &Group : MapOutput) {
----------------
nit: maybe also output the number of entries collected in the map?
================
Comment at: clang-doc/tool/ClangDocMain.cpp:181
+ doc::writeInfo(I.get(), Buffer);
+ if (DumpResultToFile("bc", Group.getKey() + ".bc", Buffer))
+ return 1;
----------------
(Sorry that I might be missing context here.)
Could you please explain the incentive for dumping each info group to one bc
file? If the key identifies a symbol (e.g. USR), wouldn't this result in a
bitcode file being created for each symbol? This doesn't seem very scalable.
https://reviews.llvm.org/D43341
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
