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

[Paul Kirth via Phabricator via cfe-commits]

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