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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
