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