[PATCH] D133732: [clang-doc] Support default args for functions.

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.

Thanks for the patch. This is mostly LGTM modulo a few nits.

My one question is regarding documentation. Do you think this needs to be described in the clang-tools-extra/doc/clang-doc.rst? And are there any changes in workflow or tool usage that we should document? I assume not, since this just changes the YAML format. Is that correct?



================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:300
 
-    auto& member = I.Members.emplace_back(
+    auto &member = I.Members.emplace_back(
         F->getTypeSourceInfo()->getType().getAsString(), F->getNameAsString(),
----------------
nit: can we avoid introducing unrelated formatting changes, here and elsewhere in the patch?


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:324-329
+      } else {
+        FieldInfo = &I.Params.emplace_back(P->getOriginalType().getAsString(),
+                                           P->getNameAsString());
       }
+    } else {
+      FieldInfo = &I.Params.emplace_back(P->getOriginalType().getAsString(),
----------------
can we combine these conditions somehow, given that they do the same thing?

Maybe this? WDYT?

```
if(!FieldInfo)
  FieldInfo = &I.Params.emplace_back(P->getOriginalType().getAsString(), ...
```



================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:452
 
-  ASTContext& Context = D->getASTContext();
+  ASTContext &Context = D->getASTContext();
   // TODO investigate whether we can use ASTContext::getCommentForDecl instead
----------------
nit: same here


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:460
   Comment->setAttached();
-  if (comments::FullComment* fc = Comment->parse(Context, nullptr, D)) {
+  if (comments::FullComment *fc = Comment->parse(Context, nullptr, D)) {
     I.Description.emplace_back();
----------------
nit: ditto


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133732/new/

https://reviews.llvm.org/D133732