paulkirth added inline comments.
================
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);
----------------
brettw wrote:
> 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.
There's nothing special regarding this particular pointer, but such checks are
common in other parts of LLVM.
We use asserts liberally throughout the LLVM codebase
(https://llvm.org/docs/CodingStandards.html#assert-liberally) and asserting
that a pointer is valid is common.
The other thing to keep in mind is that debug builds of LLVM are > 20GB, and
enabling asserts is a more desirable choice for most of our developers. That
probably isn't as bad for clang-doc, but I'd rather err on the side of caution.
While I agree that right now it may be a bit strange to have a single assert,
hopefully overtime the surrounding code will start to use such checks in a way
that is more consistent with the rest of the codebase.
But like I said, an early return is also fine IMO.
================
Comment at: clang-tools-extra/clang-doc/YAMLGenerator.cpp:195
IO.mapOptional("Access", I.Access, clang::AccessSpecifier::AS_none);
+ IO.mapOptional("Description", I.Description);
}
----------------
brettw wrote:
> 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.
Now that I see this in context, I agree that it doesn't need a comment.
================
Comment at: clang-tools-extra/unittests/clang-doc/SerializeTest.cpp:197
Method.IsMethod = true;
+
ExpectedRecordWithMethod.ChildFunctions.emplace_back(std::move(Method));
----------------
?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131298/new/
https://reviews.llvm.org/D131298
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
