brettw marked 2 inline comments as done.
brettw 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);
----------------
paulkirth wrote:
> 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.
>
Done
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
