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