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