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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth requested changes to this revision.
paulkirth added a comment.
This revision now requires changes to proceed.

I agree w/ @phosek on unit testing.

Additionally, while we don't have many of them in clang-doc, an end-to-end test would also be welcome.  In this case, it will also probably help w/ review, since we can see how clang-doc output is changing as part of the review process.  To be clear: I'm not going to block acceptance on the end-to-end tests, since that may be asking too much. It would just be helpful.

I've left more direct feedback inline.



================
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();
----------------
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?


================
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);
----------------
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.



================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:440
+  ASTContext& Context = D->getASTContext();
+  RawComment *Comment = Context.getRawCommentForDeclNoCache(D);
+  if (!Comment)
----------------
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.


================
Comment at: clang-tools-extra/clang-doc/YAMLGenerator.cpp:195
     IO.mapOptional("Access", I.Access, clang::AccessSpecifier::AS_none);
+    IO.mapOptional("Description", I.Description);
   }
----------------
Can we get a comment describing this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131298