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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clang-doc/BitcodeReader.cpp:553
+
+#define READINFO(INFO)                                                         \
+  {                                                                            \
----------------
Convert this to a template function?


================
Comment at: clang-doc/Reducer.cpp:19
+
+std::unique_ptr<Info> reduceInfos(std::vector<std::unique_ptr<Info>> &Values) {
+  return mergeInfos(Values);
----------------
Now that merging is implemented in the info library, `reduceInfos` doesn't seem to be necessary. 

 I'd suggest moving `writeInfo` closer to the bitcode writer library and get rid of the Reducer.h/cpp. 


================
Comment at: clang-doc/Reducer.cpp:23
+
+bool writeInfo(Info *I, SmallVectorImpl<char> &Buffer) {
+  llvm::BitstreamWriter Stream(Buffer);
----------------
I think a more canonical form to pass in an output buffer is via `llvm::raw_ostream`, and then callers can use `llvm::raw_string_ostream`.


================
Comment at: clang-doc/Representation.cpp:30
+
+#define ASSERT_MERGEABLE                                                       \
+  assert(IT == Other.IT && (USR == EmptySID || USR == Other.USR))
----------------
Macros are generally discouraged. Could you make this a function in `Info` e.g. `Info::mergeable(const Info &Other)`. And sub-classes can simply can `asssert(mergeable(Other))`.


================
Comment at: clang-doc/Representation.cpp:59
+  case InfoType::IT_default:
+    llvm::errs() << "Unexpected info type in index.\n";
+    return nullptr;
----------------
Use `llvm::Expected<std::unique_ptr<Info>>` (e.g. `llvm::make_error<StringError>(...)`) for error handling and let callers decide how to handle the error (e.g. `llvm::errs() << llvm::toString(Err)`).


================
Comment at: clang-doc/Representation.h:146
+protected:
+  bool mergeBase(Info &&I);
 };
----------------
juliehockett wrote:
> ioeric wrote:
> > 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.
> Sort of addressed in this update. There's an issue with where we allocate the return pointer, because we need to know the type of info at allocation time -- let me know if what's here now is too far off of what you were suggesting.
Thanks! 

I thin what you have is also fine, as long as it's easy to maintain when adding new info types. 


================
Comment at: clang-doc/Representation.h:216
 
+// A standalone function to call to merge a vector of infos into one.
+std::unique_ptr<Info> mergeInfos(std::vector<std::unique_ptr<Info>> &Value);
----------------
Please elaborate a bit on the contract e.g. all values should have the same type; otherweise ...


================
Comment at: clang-doc/Representation.h:217
+// A standalone function to call to merge a vector of infos into one.
+std::unique_ptr<Info> mergeInfos(std::vector<std::unique_ptr<Info>> &Value);
+
----------------
nit: s/Value/Values/ or Infos


================
Comment at: clang-doc/tool/ClangDocMain.cpp:181
+        doc::writeInfo(I.get(), Buffer);
+      if (DumpResultToFile("bc", Group.getKey() + ".bc", Buffer))
+        return 1;
----------------
juliehockett wrote:
> ioeric wrote:
> > (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.  
> Yes, it would. This is mostly for debugging, since there's not really any tools outside the clang system that would actually want/be able to use this information.
Ok, is there any plan to convert intermediate result to final result and output in a more accessible format? If so, please add a `FIXME` somewhere just to be clearer.


================
Comment at: clang-doc/tool/ClangDocMain.cpp:176
+    Group.getValue().clear();
+    Group.getValue().emplace_back(std::move(Reduced));
+
----------------
Rather then replace the values in `MapOutput`, it would probably be clearer if you store the reduced infos into a different container e.g. `Reduced`.


================
Comment at: docs/ReleaseNotes.rst:61
 
 - New module `abseil` for checks related to the `Abseil <https://abseil.io>`_
   library.
----------------
Are these changes relevant to this patch?


https://reviews.llvm.org/D43341