[PATCH] D48341: [clang-doc] Adding a second reduce pass

[Eric Liu via Phabricator via cfe-commits]

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