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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
