[PATCH] D45999: [clangd] Retrieve minimally formatted comment text in completion.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Looks good, nits as always and I think you want a new test case.



================
Comment at: clangd/CodeComplete.cpp:817
   Result.IncludeGlobals = true;
-  Result.IncludeBriefComments = IncludeBriefComments;
+  // We never include brief comments, they are slow and don't provide useful
+  // results for non-doxygen comments.
----------------
"include brief comments" --> "parse doxygen syntax"? Or if you prefer, "parse \brief comments"

Calling these "brief comments" is confusing as a natural reading is "a one-line comment above the function" or similar.


================
Comment at: clangd/CodeCompletionStrings.h:29
+
+/// Gets a raw documentation comment of the current active parameter
+/// of \p Result.
----------------
ilya-biryukov wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > "active" is a bit confusing - at this level we just care which arg you're interested in, right?
> > > Is this typically a substring of getDocComment for the function? If so, noting that would make this clearer.
> > This refers to the parameter that should be reported as active in `signatureHelp`.
> > Agree that it requires too much context to understand, changed to a more detailed description involving `CurrentArg` parameter.
> > 
> > 
> > Is this typically a substring of getDocComment for the function? If so, noting that would make this clearer.
> I mechanically translated the original source code without looking too much on what it does :-)
> 
> Extracting a substring from the function doc would be the behavior I expect.
> However, we simply try to get doc comment for parameter in the same way we do for functions (e.g. look for comment doc for parameter decl, without looking at function decl in the first place)
> It means we'll always get the empty doc currently. At least, I couldn't make the current code produce any docs for the parameter.
> 
> I would still keep this function, as it seems to be called in the right places. But we definitely need to parse a function comment instead.
I think this function is easy to understand in isolation, and by naming the parameter `CurrentArg` one can only really understand it in terms of its callsite in `signatureHelp`. I'd name it `ArgIndex` or similar instead, but up to you.


================
Comment at: clangd/CodeCompletionStrings.h:29
+
+/// Gets a raw documentation comment of the current active parameter
+/// of \p Result.
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > sammccall wrote:
> > > > sammccall wrote:
> > > > > "active" is a bit confusing - at this level we just care which arg you're interested in, right?
> > > > Is this typically a substring of getDocComment for the function? If so, noting that would make this clearer.
> > > This refers to the parameter that should be reported as active in `signatureHelp`.
> > > Agree that it requires too much context to understand, changed to a more detailed description involving `CurrentArg` parameter.
> > > 
> > > 
> > > Is this typically a substring of getDocComment for the function? If so, noting that would make this clearer.
> > I mechanically translated the original source code without looking too much on what it does :-)
> > 
> > Extracting a substring from the function doc would be the behavior I expect.
> > However, we simply try to get doc comment for parameter in the same way we do for functions (e.g. look for comment doc for parameter decl, without looking at function decl in the first place)
> > It means we'll always get the empty doc currently. At least, I couldn't make the current code produce any docs for the parameter.
> > 
> > I would still keep this function, as it seems to be called in the right places. But we definitely need to parse a function comment instead.
> I think this function is easy to understand in isolation, and by naming the parameter `CurrentArg` one can only really understand it in terms of its callsite in `signatureHelp`. I'd name it `ArgIndex` or similar instead, but up to you.
Ah, thanks.
I'd suggest noting that in the comment (this currently looks for comments attached to the parameter itself, and doesn't extract them from function documentation). I think most people looking at this would be interested in that detail!

Looking at the doxygen docs, it seems like they want syntax like this to work:`int foo(int x /** Doc */)`, and I'd hope we could also get this to work at some point:
```int foo(
  // Doc
  int x
);```

Not that I see much code in that style, but maybe we should write more!


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:246
   // Check documentation.
-  EXPECT_IFF(Opts.IncludeBriefComments, Results.items,
-             Contains(IsDocumented()));
----------------
You've updated the existing doxygen test cases, but I don't think you're testing the new non-doxygen functionality anywhere?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45999