[PATCH] D110823: [clangd] Add code completion of param name on /* inside function calls.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

thanks, LGTM!



================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1969
+  auto Content = llvm::StringRef(ParseInput.Contents).take_front(*Offset);
+  if (Preamble) {
+    // We are doing code completion of a comment, where we currently only
----------------
Let's drop this one as you're already checking this in `codeCompleteComment` (+ it's going to be true almost always, so it's not like this is saving time in the general case)


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1145
+  CodeCompletionTUInfo CCTUInfo;
+  std::vector<std::string> &ParamNames;
+  // For de-duplication only. StringRefs based on strings in ParamNames.
----------------
adamcz wrote:
> kadircet wrote:
> > nit: Why not directly have an `llvm::DenseSet` here? I am not sure if the order we're preserving currently is a good one anyway (depends a lot on the order of includes, i think?). If we really want to preserve some ordering, it would probably make sense to have the lexicographical one instead.
> I decided to preserve the order to match what SignatureHelp is doing, but I now realize it re-orders them anyway, so good point, fixed.
> 
> Note that I used std::set, since that works with std::string and I don't think we can safely return llvm::StringRef here.
right. you can also try `llvm::StringMap` which owns its keys.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1966
+bool shouldCodeCompleteComment(llvm::StringRef Content, unsigned &Offset,
+                               llvm::StringRef &Prefix) {
+  if (Content.empty())
----------------
adamcz wrote:
> kadircet wrote:
> > this might read easier as:
> > 
> > ```
> > while (!Contents.empty() && isAciiIdentifierContinue(Contents.back())
> >   Contents = Contents.drop_back();
> > Contents.rtrim(); // by default this drops the chars you have in `Whitespace` already.
> > Offset = Contents.size();
> > return Contents.endswith("/*");
> > ```
> > 
> > this has the different behaviour for `/* qwer ^` but I don't think we want to complete after a word anyway (that's not what we do in the rest of the completion, space terminates the previous identifier).
> > 
> > As for propagating prefix, I think it's enough to check that the file content endswith whatever name we're going to suggest in the signature help collector.
> > 
> > nit: As for `Offset`, I might actually prefer returning an `Optional<unsigned>` while renaming the function to `getFunctionArgumentCommentStart`, but i am fine with this too, up to you.
> First, returning Optional instead of bool + unsigned is better. Done.
> 
> Second, you're right that there's no need to return Prefix, it can be just as easily computed later. However, we still need it. I'm not sure what you're suggesting with checking if content ends with name. If it's literally Content.endswith(Name) that means you must have already typed the entire argument name.
> I moved the CommentPrefix computation out of this function to make the signature nicer. It's just one line anyway. We can't easily move this to the collector, since it gets the original content, meaning we'd have to pass two offsets (original completion point and the new one) so we can extract something like "fun(/*foo^);" - we need to remove everything before /* and everything after ^. Does that explanation make sense?
> 
> Third, your version is better now. Note there is no difference in behavior on "/* qwer ^". There's even a test for that. I think I ended up with that complicated version because of the Prefix extraction, and, ironically, the "/* qwer ^" case - I just confused myself too much ;-)
> 
> Thanks
Thanks!

re endswith, yeah that is definitely wrong. I actually had checking a prefix of the suggestion is a suffix of the content, but even that doesn't do the trick (Comes down to what you explained after second point). We somehow need the last (possibly empty) identifier in the contents before cursor instead.

re behaviour, I missed the special if block sitting in the middle checking for `trimmed.endswith("/*")`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110823/new/

https://reviews.llvm.org/D110823