[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Eric Liu via Phabricator via cfe-commits]

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