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

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.


================
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.
----------------
ioeric wrote:
> Shouldn't USR be `SymbolID()` by default?
It actually is garbage by default -- doing this zeroed it out.


================
Comment at: clang-doc/Representation.h:146
+protected:
+  bool mergeBase(Info &&I);
 };
----------------
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.


================
Comment at: clang-doc/tool/ClangDocMain.cpp:181
+        doc::writeInfo(I.get(), Buffer);
+      if (DumpResultToFile("bc", Group.getKey() + ".bc", Buffer))
+        return 1;
----------------
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.


https://reviews.llvm.org/D43341