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

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Oct 8 09:41:26 PDT 2021


kadircet added a comment.

thanks, mostly LG, a couple of nits :)



================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1120
+      OverloadCandidate Candidate = Candidates[I];
+      if (auto *Func = Candidate.getFunction()) {
+        if (Func->getNumParams() <= CurrentArg)
----------------
nit: early exit

```
auto *Func = ...;
if(!Func)
  continue;
...
```


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1124
+        auto *PVD = Func->getParamDecl(CurrentArg);
+        if (PVD == nullptr)
+          continue;
----------------
nit: `!PVD` same for `Ident`


================
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.
----------------
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.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1965
+// run) and Prefix is the already existing prefix inside this comment.
+bool shouldCodeCompleteComment(llvm::StringRef Content, unsigned &Offset,
+                               llvm::StringRef &Prefix) {
----------------
let's name it more explicitly that this only checks if user is likely to be completing for function argument comment.

`maybeFunctionArgumentComment` ?

documentation just needs to reflect that fact now rather than explaining the being only type of comment completion supported and such.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1966
+bool shouldCodeCompleteComment(llvm::StringRef Content, unsigned &Offset,
+                               llvm::StringRef &Prefix) {
+  if (Content.empty())
----------------
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.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1956
+  auto Content = llvm::StringRef(ParseInput.Contents).take_front(*Offset);
+  if (Content.endswith("/*")) {
+    // We are doing code completion of a comment, where we currently only
----------------
adamcz wrote:
> kadircet wrote:
> > what if the user triggers at `foo(/*  b^`? I think we'd still want to provide `bar`.
> > 
> > I think we should detect whether we're in a comment first, we can run the Lexer for that but it'd probably be an overkill. For now I think we can just check for the string after last `*/`, if it has a `/*` we're in a comment, otherwise we check for the current line only. if it has a `//` we're again in a comment.
> > 
> > Then we can switch to the `codeCompleteComment` mode, inside there one branch can collect completion results for possible parameter names (while filtering using the currently typed text) and we can expand with more logic later on (e.g. suggest identifiers from index?).
> > 
> > WDYT?
> Good point about /* b^, thanks! I did not consider manually triggered code completion at all.
> 
> Detecting last */ the way you suggest seems fragile. There's macros that can expand to /* or */, there's strings, lines that are already comments (i.e. "// blah /* blah), "#if 0" sections, etc.
> Running Lexer is an option, but I don't think it's necessary. Currently there's very few options when we can offer code completion:
> /*^
> /*   ^
> /*foo^
> /*   foo^
> This can be quickly detected by scanning back through whitespace, then valid identifier characters, then whitespace again.
> 
> Since it's unclear if and when we'll be adding more code completion in comments I want to keep this simple rather than build for the future when we may indeed need a lexer.
> 
> WDYT?
> 
> (also added some tests to cover this).
As discussed offline, I was mostly worried about extending this in the future, but we can think about that later. So this is good as-is.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1963
+    auto OffsetBeforeComment = *Offset - 2;
+    return codeCompleteComment(FileName, OffsetBeforeComment, Preamble,
+                               ParseInput);
----------------
adamcz wrote:
> kadircet wrote:
> > i think we should propagate `Opts` here and then override the pieces accordingly.
> Hmm...why?
> 
> It seems that everything that could be relevant would be overridden here. SignatureHelp, which is very similar to this, doesn't use code complete options either. I think it may be confusing to accept an object but then override literally every single thing that matters anyway.
> 
> It's not unlikely that in the future we'll need options, but for now it seems useless. Do you have a specific option you want propagated?
ah nvm. I confused `clang::CodeCompleteOptions` with `clangd`s


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110823



More information about the cfe-commits mailing list