[PATCH] D140275: [clangd] Tweak to add doxygen comment to the function declaration

Wed Jan 11 11:45:25 PST 2023

sammccall added a comment.

Sorry about the delay on this.
This looks useful - I know a lot of people write comments in this style, whatever my own reservations :-)

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:30
+// Given:
+//   int func(int x, char const* s, Bar bar) {}
+// the tweak add the doxygen comment to the function in the form
----------------
nit: mark where triggering is possible with ^^s on the next line (see docs for other tweaks)

IMO we should only be triggering on `func` itself - tweaks that fire on a large portion of the codebase reduce the signal/noise ratio too much.

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:32
+// the tweak add the doxygen comment to the function in the form
+//   /*!
+//    * @brief
----------------
`///`-style comments appear to be the most common style, followed fairly closely by `/**`, with `/*!` a distant third. (1.4M vs 1.1M vs 0.2M files in our internal codebase: we don't use doxygen comments so it's a sample of third-party libraries).

`///` is also the style that LLVM prefers: https://llvm.org/docs/CodingStandards.html#comment-formatting

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:33
+//   /*!
+//    * @brief
+//    * @param x
----------------
I'd reluctantly ask to remove `@brief` from the template. It hurts readability too much for people reading the code (as opposed to reading generated docs).

I realize that doxygen no longer extracts any part of the comment by default, but I think it's up to them to fix their defaults.

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:37
+//    * @param bar
+//    * @return
+//    */
----------------
I'm a bit concerned about people generating these `@param bar` and `@return` and leaving them there without filling them in - I've seen plenty of code like that and it's substantially worse than no comments at all.

I'm not sure we can do much though: could generate `@return TODO` or so to make it more visually obvious - WDYT?

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:58
+};
+
+REGISTER_TWEAK(AddDoxygenComment)
----------------
tschuett wrote:
> The LLVM coding style kindly asks you stop the anonymous namespace here. Furthermore, I believe that the `REGISTER_TWEAK` does not work in an anonymous namespace.
The coding style is sorely outdated on this point (indentation?!), and the code in this subproject doesn't follow it - happy to take this up on discourse if needed but I don't think we should create a mess of mixed local style here.

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:62
+std::string AddDoxygenComment::title() const {
+  return "Add doxygen comment to the function declaration";
+}
----------------
"Add documentation comment"?

Short is quicker to scan, and if we keep the triggering tight then it's obvious what it's for.

I suspect if people care much about the flavor of doc comment, we'll end up adding config to customize it (I can see it coming with `///` vs `//` vs `/**` vs `/*!` already...) and using a generic name is going to work better than trying to describe the style.

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:65
+
+bool AddDoxygenComment::isCommentExist(FunctionDecl const *FD) {
+  if (RawComment *Comment =
----------------
west const in llvm please (and other occurrences elsewhere)

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:65
+
+bool AddDoxygenComment::isCommentExist(FunctionDecl const *FD) {
+  if (RawComment *Comment =
----------------
sammccall wrote:
> west const in llvm please (and other occurrences elsewhere)
isCommentExist => hasComment

but really this is just Inputs.AST->getRawCommentForDeclNoCache(D) != nullptr, maybe just inline it?

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:74
+bool AddDoxygenComment::prepare(const Selection &Inputs) {
+  if (!Inputs.AST->getLangOpts().CPlusPlus) {
+    return false;
----------------
why? doxygen supports C AFAIK

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:78
+  if (auto *N = Inputs.ASTSelection.commonAncestor()) {
+    if (FunctionDecl const *FD = N->ASTNode.get<FunctionDecl>()) {
+      if (isCommentExist(FD)) {
----------------
This may be the body of a function template, in which case we could still generate doc but it needs to be inserted above the TemplateDecl, not above the FunctionDecl.

(feel free to bail out in this case though)

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:85
+    }
+    for (auto *P = N->Parent; P != nullptr; P = P->Parent) {
+      if (FunctionDecl const *FD = P->ASTNode.get<FunctionDecl>()) {
----------------
I don't think we should be walking up, code actions should really be relevant to the thing the user is targeting, and I don't think e.g. pointing at a param means you want to act on the function.

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:107
+  for (auto const *P : FD->parameters()) {
+    OS << "* @param " << P->getName() << '\n';
+  }
----------------
what about unnamed parameters? I think we can't usefully document them, should we skip them here or disallow the code action entirely?

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:120
+  auto &SrcMgr = Inputs.AST->getSourceManager();
+  std::vector<Anchor> Anchors = {{[this](Decl const *D) {
+                                    if (auto const *FD =
----------------
insertionPoint() is used when adding decls to a scope where we don't know the existing set.

Here we already have the function, and just want to insert before it.
can't we use FD->getSourceRange()?

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:132
+  if (!Parent) {
+    return error("Parent for the FuncDecl is nullptr");
+  }
----------------
these are user-visible errors, we shouldn't use AST jargon here

================
Comment at: clang-tools-extra/clangd/unittests/tweaks/AddDoxygenCommentTests.cpp:1
+//===-- AddDoxygenCommentTests.cpp ------------------------*- C++ -*-===//
+//
----------------
There are some more cases to consider, which should be tested:

do we do anything different if:

- this is a forward-declaration of a function? (i don't think so)
- this is a definition of a function that was previously forward-declared? (I think the main file is not a header and there's a declaration in a header we probably shouldn't offer the tweak - it's confusing to insert somewhere else but wrong to insert here)
- this is an in-class declaration of a member function (i don't think so)
- this is an out-of-line definition of a member function (I think it's always wrong to put a doxygen comment here)
 - this is a function template (fine to insert, but positioning needs care)
 - the "function" is a deduction guide (we shouldn't insert)

================
Comment at: clang-tools-extra/clangd/unittests/tweaks/AddDoxygenCommentTests.cpp:41
+
+TEST_F(AddDoxygenCommentTest, Apply1) {
+  EXPECT_EQ(apply(R"cpp(
----------------
rather than 1/2 please give these somewhat descriptive names

================
Comment at: clang-tools-extra/clangd/unittests/tweaks/AddDoxygenCommentTests.cpp:53
+  
+/*!
+ * @brief
----------------
the whitespace here is wonky - it's not critical to get the indentation right, but it should be consistent

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140275