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