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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/CodeCompletionStrings.h:29
+
+/// Gets a raw documentation comment of the current active parameter
+/// of \p Result.
----------------
sammccall wrote:
> 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!
Noting that is definitely useful. And yes, ideally we should support both styles.


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:246
   // Check documentation.
-  EXPECT_IFF(Opts.IncludeBriefComments, Results.items,
-             Contains(IsDocumented()));
----------------
sammccall wrote:
> You've updated the existing doxygen test cases, but I don't think you're testing the new non-doxygen functionality anywhere?
Yep, I moved all the tests to clang when reviewing the patch. I'll add more tests to the last change that actually sets ParseAllComments to true.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45999