[clangd] Tweak to add doxygen comment to the function declaration

[tupos]

I do not know but maybe it makes sence to make configurable the style of the comment, e.g instead of @brief use \brief and the same for /*! to /**

[tschuett]

clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp

24

You can merge this into namespace clang::clangd.

58

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.

[tupos]

Also I tried to use it now, and I'm not sure where it is a good idea to allow this tweak from within a function body. Because it is quite annoying that in every function you get this code action.

On the other hand maybe it is ok to allow it from within the body, but the tweak then should be smarter and should check whether on the function declaration the doxygen comment is already present. Also if the tweak is smarter it is fine to get this code action, as it will motivate me to get rid of it and provide a proper documentation ;-).

What do you think?

Another question that I have, whether it makes sense to add @pre and @post to the default snippet.

[sammccall]

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

clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp

30

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.

32

///-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

33

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.

37

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?

58

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.

62

"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.

65

west const in llvm please (and other occurrences elsewhere)

65

isCommentExist => hasComment

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

74

why? doxygen supports C AFAIK

78

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)

85

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.

107

what about unnamed parameters? I think we can't usefully document them, should we skip them here or disallow the code action entirely?

120

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()?

132

these are user-visible errors, we shouldn't use AST jargon here

clang-tools-extra/clangd/unittests/tweaks/AddDoxygenCommentTests.cpp

1

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)

41

rather than 1/2 please give these somewhat descriptive names

53

the whitespace here is wonky - it's not critical to get the indentation right, but it should be consistent

[dgoldman]

clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp

37

VS Code itself has support for snippets - https://code.visualstudio.com/api/references/vscode-api#TextEditor - insertSnippet - but the LSP spec doesn't yet. Once it has it I think it would makes sense to use them here, but until then, TODO seems like the best we can do?

74

Would also be nice to support ObjC too, we'll just need to add support for ObjCMethodDecl as well to expand support for ObjC methods. (https://clang.llvm.org/doxygen/classclang_1_1ObjCMethodDecl.html vs https://clang.llvm.org/doxygen/classclang_1_1FunctionDecl.html) But even if that's not done in this diff, still seems fine to enable it generally?

[dgoldman]

Hey, are you still planning to work on this? Otherwise, is it okay if I take over this patch to implement this feature?

[tupos]

Hi,

sorry for the long delay, but actually in this particular moment I was working on addressing the comments.

Therefore, I will push the updated patch soon.

[tupos]

clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp

24

I prefer not to do it, because all other files with tweaks have the same style.
However, if it is still necessary please let me know and I will change it.

37

I do not think that adding here TODO makes much sense, because in this case I can just type the whole comment automatically. Think about I added a template but then I need to remove part of this template because it was a placeholder.

I personally think that people who do not want to fill the template properly should blame themselves for not writing a proper comment and it is not a responsibility of the clangd to motivate them to do it.

What I'm not sure about whether we need to add to the template @pre and @post because this is more important as filling the parameters names.

Ideally it would be good if the template can be configurable through the .clangd config. What do you think?

58

See the comment above.

74

What is the correct language option for the C language? I found only the enumeration of all C standards?

[tupos]

Could you please also advice me what else need to be done for the ObjC, since there were many years since I wrote ObjC last time I'm not sure what else need to be done there.

Thanks.

[tupos]

Hi,

could you please provide a code review again?

Thanks.

With best regards,

[dgoldman]

In D140275#4203456, @tupos wrote:

Could you please also advice me what else need to be done for the ObjC, since there were many years since I wrote ObjC last time I'm not sure what else need to be done there.

Thanks.

I think this is fine for a v1 - I think the main improvement would be changing the style/format, Xcode itself uses the following format instead where <#Placeholder Name#> denotes a placeholder:

/// <#Description#>
/// - Parameters:
///   - arg1: <#arg1 description#>
///   - arg2: <#arg2 description#>
/// - Returns: <#description#>

clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp

+//===--- AddDoxydgenComment.cpp ---------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#include "AST.h"
+#include "Selection.h"
+#include "refactor/InsertionPoint.h"
+#include "refactor/Tweak.h"
+#include "support/Logger.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/raw_ostream.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+// A tweak that adds a doxygen comment to the function declaration
+//
+// Given:
+//   int func(int x, char const* s, Bar bar) {}
+// the tweak add the doxygen comment to the function in the form
+//   /*!
+//    * @brief
+//    * @param x
+//    * @param s
+//    * @param bar
+//    * @return
+//    */
+//   int func(int x, char const* s, Bar bar) {}
+//   if the function return type is void no @return is added to the doxygen
+//   comment
+//
+class AddDoxygenComment : public Tweak {
+public:
+  const char *id() const final;
+  llvm::StringLiteral kind() const override {
+    return CodeAction::REFACTOR_KIND;
+  }
+  bool prepare(const Selection &Inputs) override;
+  Expected<Effect> apply(const Selection &Inputs) override;
+  std::string title() const override;
+
+private:
+  FunctionDecl const *FD;
+  std::string buildCommentCode() const;
+  static bool isCommentExist(FunctionDecl const *FD);
+};
+
+REGISTER_TWEAK(AddDoxygenComment)
+
+std::string AddDoxygenComment::title() const {
+  return "Add doxygen comment to the function declaration";
+}
+
+bool AddDoxygenComment::isCommentExist(FunctionDecl const *FD) {
+  if (RawComment *Comment =
+          FD->getASTContext().getRawCommentForDeclNoCache(FD)) {
+    return true;
+  }
+  return false;
+}
+
+bool AddDoxygenComment::prepare(const Selection &Inputs) {
+  if (!Inputs.AST->getLangOpts().CPlusPlus) {
+    return false;
+  }
+  if (auto *N = Inputs.ASTSelection.commonAncestor()) {
+    if (FunctionDecl const *FD = N->ASTNode.get<FunctionDecl>()) {
+      if (isCommentExist(FD)) {
+        return false;
+      }
+      this->FD = FD;
+      return true;
+    }
+    for (auto *P = N->Parent; P != nullptr; P = P->Parent) {
+      if (FunctionDecl const *FD = P->ASTNode.get<FunctionDecl>()) {
+        if (isCommentExist(FD)) {
+          return false;
+        }
+        this->FD = FD;
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
+std::string AddDoxygenComment::buildCommentCode() const {
+  std::string Res;
+  llvm::raw_string_ostream OS(Res);
+  OS << R"cpp(
+/*!
+ * @brief
+ *
+)cpp";
+  for (auto const *P : FD->parameters()) {
+    OS << "* @param " << P->getName() << '\n';
+  }
+  if (auto const *RT = FD->getReturnType().getTypePtr()) {
+    if (!RT->isVoidType()) {
+      OS << "* @return\n";
+    }
+  }
+  OS << "*/\n";
+  return Res;
+}
+
+Expected<Tweak::Effect> AddDoxygenComment::apply(const Selection &Inputs) {
+  auto &SrcMgr = Inputs.AST->getSourceManager();
+  std::vector<Anchor> Anchors = {{[this](Decl const *D) {
+                                    if (auto const *FD =
+                                            llvm::dyn_cast<FunctionDecl>(D)) {
+                                      if (FD == this->FD) {
+                                        return true;
+                                      }
+                                    }
+                                    return false;
+                                  },
+                                  Anchor::Above}};
+  auto const *Parent = FD->getParent();
+  if (!Parent) {
+    return error("Parent for the FuncDecl is nullptr");
+  }
+  // we cannot use here insertDecl as if the Parent is TranslationUnitDecl
+  // it will fail, as TranslationUnitDecl does not have a valid location
+  auto Loc = insertionPoint(*Parent, Anchors);
+  if (Loc.isInvalid()) {
+    return error("Location for Declatation {0} is invalid",
+                 Parent->getDeclKindName());
+  }
+  tooling::Replacement Replacement(SrcMgr, Loc, 0, buildCommentCode());
+  return Effect::mainFileEdit(SrcMgr,
+                              tooling::Replacements{std::move(Replacement)});
+}
+} // namespace
+} // namespace clangd
+} // namespace clang

clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt

 include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../..)
 
 set(LLVM_LINK_COMPONENTS
   support
   )
 
 # A target containing all code tweaks (i.e. mini-refactorings) provided by
 # clangd.
 # Built as an object library to make sure the linker does not remove global
 # constructors that register individual tweaks in a global registry.
 # To enable these tweaks in executables or shared libraries, add
 # $<TARGET_OBJECTS:obj.clangDaemonTweaks> to a list of sources, see
 # clangd/tool/CMakeLists.txt for an example.
 add_clang_library(clangDaemonTweaks OBJECT
+  AddDoxygenComment.cpp
   AddUsing.cpp
   AnnotateHighlightings.cpp
   DumpAST.cpp
   DefineInline.cpp
   DefineOutline.cpp
   ExpandAutoType.cpp
   ExpandMacro.cpp
   ExtractFunction.cpp

clang-tools-extra/clangd/unittests/CMakeLists.txt

add_unittest(ClangdUnitTests ClangdTests
   support/FunctionTests.cpp
   support/MarkupTests.cpp
   support/MemoryTreeTests.cpp
   support/PathTests.cpp
   support/ThreadingTests.cpp
   support/TestTracer.cpp
   support/TraceTests.cpp
 
+  tweaks/AddDoxygenCommentTests.cpp
   tweaks/AddUsingTests.cpp
   tweaks/AnnotateHighlightingsTests.cpp
   tweaks/DefineInlineTests.cpp
   tweaks/DefineOutlineTests.cpp
   tweaks/DumpASTTests.cpp
   tweaks/DumpRecordLayoutTests.cpp
   tweaks/DumpSymbolTests.cpp
   tweaks/ExpandAutoTypeTests.cpp

clang-tools-extra/clangd/unittests/tweaks/AddDoxygenCommentTests.cpp

+//===-- AddDoxygenCommentTests.cpp ------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TweakTesting.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TWEAK_TEST(AddDoxygenComment);
+
+TEST_F(AddDoxygenCommentTest, AvailableUnavailable) {
+  EXPECT_AVAILABLE(R"cpp(
+    void bar(char b) {^ }
+    [[int foo(int x) { }]]
+    namespace ns1 {
+      void bar(char b) {^ }
+      [[int foo(int x) { }]]
+    }
+  )cpp");
+  EXPECT_UNAVAILABLE(R"cpp(
+    namespace ns1 {
+      /*!
+       */
+      void bar(char b) {^ }
+      // comment
+      [[int foo(int x) { }]]
+      /// comment
+      [[int meaw(int x) { }]]
+    }
+  )cpp");
+}
+
+TEST_F(AddDoxygenCommentTest, Apply1) {
+  EXPECT_EQ(apply(R"cpp(
+namespace ns1 {
+  int foo(int y, char* s) {
+    ^int x;
+    return x;
+  }
+}
+  )cpp"),
+            R"cpp(
+namespace ns1 {
+  
+/*!
+ * @brief
+ *
+* @param y
+* @param s
+* @return
+*/
+int foo(int y, char* s) {
+    int x;
+    return x;
+  }
+}
+  )cpp");
+}
+
+TEST_F(AddDoxygenCommentTest, Apply2) {
+  EXPECT_EQ(apply(R"cpp(
+void foo();
+void ^bar();
+namespace ns1 {
+  int foo(int y, char* s) {
+    int x;
+    return x;
+  }
+}
+)cpp"),
+            R"cpp(
+void foo();
+
+/*!
+ * @brief
+ *
+*/
+void bar();
+namespace ns1 {
+  int foo(int y, char* s) {
+    int x;
+    return x;
+  }
+}
+)cpp");
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang