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