[PATCH] D41102: Setup clang-doc frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett planned changes to this revision.
juliehockett added inline comments.


================
Comment at: tools/clang-doc/ClangDoc.cpp:60
+
+comments::FullComment *ClangDocVisitor::getComment(const Decl *D) {
+  RawComment *Comment = Context->getRawCommentForDeclNoCache(D);
----------------
jakehehrlich wrote:
> Can this be a const method?
Not right now -- it's actually updating the `Attached` attribute of the comment, since it's not actually set in the initial parsing. It should be moved out into the initial comment parsing (see FIXME), but that's a separate patch. I should probably write that. :)


================
Comment at: tools/clang-doc/ClangDoc.cpp:123
+ClangDocAction::CreateASTConsumer(CompilerInstance &C, StringRef InFile) {
+  return make_unique<ClangDocConsumer>(&C.getASTContext(), Reporter);
+}
----------------
jakehehrlich wrote:
> Pro Tip: Always explicitly refer to this as "llvm::make_unique" because you'll have to revert this change if you don't.
> 
> Some of the build bots have C++14 headers instead of C++11 headers. This means that llvm::make_unique and std::make_unique will both be defined. This means that using "make_unique" will cause an error even though only llvm::make_unique can be referred to unqualified. So even if you're inside of the llvm namespace you should explicitly refer to "llvm::make_unique" and never use "make_unique".
Oh interesting -- thanks for the tip!


================
Comment at: tools/clang-doc/ClangDoc.h:29
+struct ClangDocContext {
+  // Which format in which to emit representation.
+  OutFormat EmitFormat;
----------------
sammccall wrote:
> Is this the intermediate representation referred to in the design doc, or the final output format?
> 
> If the former, why two formats rather than picking one?
> YAML is nice for being usable by out-of-tree tools (though not as nice as JSON). But it seems like providing YAML as a trivial backend format would fit well?
> Bitcode is presumably more space-efficient - if this is significant in practice it seems like a better choice.
That's the idea -- for developing purposes, I wrote up the YAML output first for this patch, and there will be a follow-on patch expanding the bitcode/binary output. I've updated the flags to default to the binary, with an option to dump the yaml (rather than the other way around).


================
Comment at: tools/clang-doc/ClangDoc.h:33
+
+class ClangDocVisitor : public RecursiveASTVisitor<ClangDocVisitor> {
+public:
----------------
jakehehrlich wrote:
> sammccall wrote:
> > This API makes essentially everything public. Is that the intent?
> > 
> > It seems like `ClangDocVisitor` is a detail, and the operation you want to expose is "extract doc from this AST into this reporter" or maybe "create an AST consumer that feeds this reporter".
> > 
> > It would be useful to have an API to extract documentation from individual AST nodes (e.g. a Decl). But I'd be nervous about trying to use the classes exposed here to do that. If it's efficiently possible, it'd be nice to expose a function.
> > (one use case for this is clangd)
> Correct me if I'm wrong but I believe that everything needs to be public in this case because the base class needs to be able to call them. So the visit methods all need to be public.
Yes to the `Visit*Decl` methods being public because of the base class.

That said, I shifted a few things around here and implemented it as a `MatcherFinder` instead of a `RecursiveASTVisitor`. The change will allow us to make most of the methods private, and have the ability to fairly easily implement an API for pulling a specific node (e.g. by name or by decl type). As far as I understand (and please correct me if I'm wrong), the matcher traverses the tree in a similar way. This will also make mapping through individual nodes easier.


================
Comment at: tools/clang-doc/ClangDoc.h:39
+
+  bool VisitTagDecl(const TagDecl *D);
+  bool VisitNamespaceDecl(const NamespaceDecl *D);
----------------
sammccall wrote:
> `override` where applicable
I might be wrong, but I don't believe the Visit*Decl methods are overrides for RecursiveASTVisitor?


================
Comment at: tools/clang-doc/ClangDocReporter.h:48
+/// A representation of a parsed comment.
+struct CommentInfo {
+  std::string Kind;
----------------
jakehehrlich wrote:
> There are a lot of std::string members here, do we know for sure that we CommentInfo to own all of these? My general strategy is to avoid owning data (e.g. use StringRef and ArrayRef) unless there's a good reason to own the data. This is a general question I have about all FooInfo structs.
So the issue here is that most of this data is owned by the various Decl things, which will go out of scope before the data is serialized. That said, I think this won't be a problem at all once I refactor the intermediate output for mapping and reducing instead of doing it in memory.


================
Comment at: tools/clang-doc/ClangDocReporter.h:190
+
+  std::shared_ptr<CommentInfo> CurrentCI;
+  Documentation Docs;
----------------
jakehehrlich wrote:
> This seems like a code smell to me. I haven't read though the usage of CurrentCI well enough yet to properly conclude what to do about this but I have an idea. Do you think it's possible to have another class that includes the methods that use CurrentCI? The other alternative might be to pass this just to where it's needed as that's basically what you're doing.
> 
> This all said, I haven't read most of this code yet so feel free to disregard this for right now.
I tidied this up a little bit by breaking out the CommentVisitor into its own class and returning unique_ptrs, but it's not ideal still because the class itself uses a raw pointer to actually build the comment. The issue is that the `visit*Comment` methods are all called from the base `ConstCommentVisitor` traverse method, and so the data structure can't be passed around there...


https://reviews.llvm.org/D41102