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

[Oleg Skoromnik via Phabricator via cfe-commits]

tupos added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:24
+namespace clang {
+namespace clangd {
+namespace {
----------------
tschuett wrote:
> You can merge this into `namespace clang::clangd`.
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.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:37
+//    * @param bar
+//    * @return
+//    */
----------------
dgoldman wrote:
> sammccall wrote:
> > 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?
> 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?
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?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:58
+};
+
+REGISTER_TWEAK(AddDoxygenComment)
----------------
sammccall wrote:
> 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.
See the comment above.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:74
+bool AddDoxygenComment::prepare(const Selection &Inputs) {
+  if (!Inputs.AST->getLangOpts().CPlusPlus) {
+    return false;
----------------
dgoldman wrote:
> sammccall wrote:
> > why? doxygen supports C AFAIK
> 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?
What is the correct language option for the C language? I found only the enumeration of all C standards?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140275