[PATCH] D131298: [clang-doc] Read docstrings for record members

[Brett Wilson via Phabricator via cfe-commits]

brettw added a comment.





================
Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:354
+template <> llvm::Expected<CommentInfo *> getCommentInfo(MemberTypeInfo *I) {
+  I->Description.emplace_back();
+  return &I->Description.back();
----------------
paulkirth wrote:
> So, I see that this uses the same pattern as in other `getCommentInfo(...)` API's, but I'm a bit confused by this.
> 
> Do we always grow the vector whenever `getCommentInfo(...)` is  called? I would expect a `get...` function to be `const` and just return data. This grows the `Description` vector on every call, which seems like an odd choice. The pattern is also pervasive in BitcodeReader.cpp.
> 
> @phosek is this intentional? If `Description` exceeds capacity and reallocs on `emplace_back`, then any reference it had returned would be invalidated, right? Or am I missing something here re: the various `*TypeInfos` and how clang-doc uses them?
You are correct that this always grows. These functions are only called once and that's when readSubBlock gets a block ID. The comment is filled into the returned array.

My question is why the comments on the info structs are arrays. I have only ever seen one top-level comment of type "FullComment" which can have many children.


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:439
+static void populateMemberTypeInfo(MemberTypeInfo &I, const FieldDecl *D) {
+  ASTContext& Context = D->getASTContext();
+  RawComment *Comment = Context.getRawCommentForDeclNoCache(D);
----------------
paulkirth wrote:
> I think we need to at least assert that `D != nullptr`. While there is only one caller now, its likely there may be more in the future. Plus `nullptr` is deref is never fun to debug.  
> 
> If getting a `nullptr` is expected to be a common input, then an early return is also fine.
> 
> If you prefer, a few places in the codebase do both to maintain correct behavior when asserts are disabled. Often they have a form along these lines:
> 
> ```
> if(!D){
>   assert(false && "Some error message");
>   return;
> }
> ```
> 
> Any of these would be fine, and I don't have a preference between them.
> 
I'm fine adding an assert to unblock the review.

But clang-doc is full of pointers and none of them are asserted on. Is this one special in some way?  To me, it seems weird to assert on this one random pointer. Is there some policy about this?

I follow the opposite theory that asserts on null pointers all over the place clutter the code and random null parameters are one of the easiest types of crashes to debug.


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:440
+  ASTContext& Context = D->getASTContext();
+  RawComment *Comment = Context.getRawCommentForDeclNoCache(D);
+  if (!Comment)
----------------
paulkirth wrote:
> If the goal is to get the FullComment, then doesn't `getCommentForDecl(...) ` handle that? This also seems like an area where we'd want to use caching if we could.
I have no idea how this works, I copied this snipped from MapASTVisitor::getComment in Mapper.cpp


================
Comment at: clang-tools-extra/clang-doc/YAMLGenerator.cpp:195
     IO.mapOptional("Access", I.Access, clang::AccessSpecifier::AS_none);
+    IO.mapOptional("Description", I.Description);
   }
----------------
paulkirth wrote:
> Can we get a comment describing this?
I don't think this needs a comment, a substantial part of this file is just adding these mappings between YAML keys and field names. The one above it has a comment only because of the non-obvious default value. If you feel strongly, please let me know what the comment should say.


================
Comment at: clang-tools-extra/unittests/clang-doc/BitcodeTest.cpp:291
+/*
+TEST(BitcodeTest, emitMemberTypeInfo) {
+  MemberTypeInfo I;
----------------
Whoops, I'll delete this.


================
Comment at: clang-tools-extra/unittests/clang-doc/SerializeTest.cpp:166
+  ExpectedE.Members.emplace_back("int", "value", AccessSpecifier::AS_public);
+  //ExpectedE.Members.back().Description.push_back(MakeOneLineCommentInfo(" Some docs"));
   CheckRecordInfo(&ExpectedE, E);
----------------
I spent quite some time trying to get this to work and I'm stuck. If I copy the contents of the raw string above into a file and run clang-doc on it, I get a comment in the YAML output. But in this test I never get a comment and I don't know how this parsing environment is different than the "real" one.

I'm very confused by the result of `ExtractInfosFromCode` function in the first place. What are the 1st and 6th item which are never checked? Why does it emit two duplicate records for the `E` struct, one (Infos[2]) with a data member and no function and one (Infos[3]) with no data member and a function in it?

Any suggestions here would be appreciated.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131298/new/

https://reviews.llvm.org/D131298