ioeric added a comment.
Thanks for the refactoring and the comments! They made it a lot easier to
understand the changes.
I'm focusing on how the changes would fit into the `ToolExecutor` framework in
the review and will leave tool-specific logics to another reviewer who I assume
would know the tool better than me :)
================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:121
llvm::Expected<llvm::SmallString<128>>
getPath(StringRef Root, StringRef Ext, StringRef Name,
llvm::SmallVectorImpl<doc::Reference> &Namespaces) {
----------------
ioeric wrote:
> This deserves a comment. It's hard to tell what this does without looking at
> the implementation.
So it would probably be clearer to call this `getInfoOutputFile`?
Given how path is created, it might make more sense if the parameter order is
`(Root, Namespaces, Name, Ext)`
================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:124
+//
+// Example: Given the below, the yaml path for class C will be
<root>/A/B/C.yaml
+//
----------------
nit: s/yaml/<Ext>/?
================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:160
+void addToInfoPassOutput(
+ StringRef Key, std::unique_ptr<doc::Info> &&I,
----------------
Now that this only has one call site, it might be clearer to just inline this.
================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:167
+
+bool mapResultsInMemory(
+ tooling::ToolResults &Results,
----------------
`mapResultsInMemory` doesn't seem to describe what this does. Maybe
`bitcodeResultsToInfos`? Please also add comment.
================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:238
+ llvm::outs() << "Collecting infos...\n";
+ llvm::StringMap<std::vector<std::unique_ptr<doc::Info>>> MapOutput;
+ if (mapResultsInMemory(*Exec->get()->getToolResults(), MapOutput))
----------------
nit: maybe `USRToInfos`?
================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:244
+ llvm::outs() << "Reducing " << MapOutput.size() << " infos by name...\n";
+ tooling::InMemoryToolResults ReduceResults;
for (auto &Group : MapOutput) {
----------------
The `ToolResults` abstraction is intended to be used by `ToolExecutor`
framework only. It seems that what you want is just a `StringMap`?
================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:262
+ // Prepare for second reduce pass by re-mapping infos by enclosing scope.
+ if (auto Error =
----------------
If what we want is a mapping from enclosing scope to infos, wouldn't it be
easier if we make the mapper (the `ToolAction` in `execute`) collect
`Scope->Infos` (instead of `USR`->`Infos`) in the first place?
================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:276
+
+ // Second reducing phase (by scope) and docs generation phase
+ // This pass reduces the re-mapped infos by enclosing scope, meaning that all
----------------
Is there any reason why this needs to be a separate reduce pass? It seems that
it should be possible to merge two reduce passes e.g. by making `mergeInfos`
incremental. Having two reduce passes would diverge further from the potential
MapReduce execution.
================
Comment at: clang-tools-extra/test/clang-doc/yaml-comments.cpp:30
+// CHECK: ---
+// CHECK-NEXT: USR:
'{{[0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z]}}'
+// CHECK-NEXT: ChildFunctions:
----------------
Hmm, is this the same as `[0-9A-Z]{n}` ?
https://reviews.llvm.org/D48341
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
