This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add option to fold overloads into a single completion item.
ClosedPublic

Authored by sammccall on Jun 8 2018, 12:30 PM.

Details

Summary

Adds a CodeCompleteOption to folds together compatible function/method overloads into a single item. This feels pretty good (for editors with signatureHelp support), but has limitations.

This happens in the code completion merge step, so there may be inconsistencies (e.g. if only one overload made it into the index result list, no folding).

We don't want to bundle together completions that have different side-effects (include insertion), because we can't constructo a coherent CompletionItem. This may be confusing for users, as the reason for non-bundling may not be immediately obvious. (Also, the implementation seems a little fragile)

Event Timeline

sammccall created this revision.Jun 8 2018, 12:30 PM
sammccall updated this revision to Diff 151315.Jun 14 2018, 2:31 AM

Turn prototype into something we could ship:

  • move bundling behind an option (off by default)
  • don't fold together items if they may insert different headers
  • clean up code and add tests
sammccall retitled this revision from [clangd] Prototype of overload folding to [clangd] Add option to fold overloads into a single completion item..Jun 14 2018, 2:33 AM
sammccall edited the summary of this revision. (Show Details)
sammccall added a reviewer: ilya-biryukov.
sammccall updated this revision to Diff 151316.Jun 14 2018, 2:35 AM

Give ScoredBundleGreater a better name/location.

Plan regarding hiding signatures, and configuration:

  • internally, keep these options independent (programmatic users know what they're doing)
  • as a CLI flag, make it a tristate (detailed/bundled/simple), because !Bundle && !Signatures doesn't make sense.

Not 100% sure on this (particularly flag naming), WDYT?

Oh, sorry, I forgot to submit the comments yesterday :-(

clangd/CodeComplete.cpp
245

Members never come from the index for completion, right? Maybe add an assert here instead?

265

getQualifiedNameAsString has a fixme, suggesting it's deprecated. Maybe we should call printName directly instead? And we have Scratch for storage anyway, so it might be a little faster too.

366

Maybe use a unicode ellipsis char (…) here? Or even come up with something else. ... is a valid syntax in C++, might be little confusing. Especially in VSCode, where detail is not shown.

Overall LG. It would be nice to mention somewhere that we since we bundle overloads client-side we might run into weird side-effects (e.g. return less completion items than the specified limit) in case the index returns too many non-bundled.

clangd/tool/ClangdMain.cpp
62

NIT: maybe use FIXME to be consistent with the rest of clangd?

sammccall marked 4 inline comments as done.Jun 15 2018, 2:57 AM
sammccall added inline comments.
clangd/CodeComplete.cpp
245

Done.
I'd keep the actual return here as it's simple and explains the value. We likely want to use it in future to augment members with signals from the index.

sammccall updated this revision to Diff 151476.Jun 15 2018, 2:58 AM
sammccall marked an inline comment as done.

Review comments

ilya-biryukov accepted this revision.Jun 15 2018, 3:25 AM

LGTM. (and a question on why we want to special-case the members)

clangd/CodeComplete.cpp
245

LG, thanks!

263

Using the normal-function scheme (fully-qualified name as grouping key) should also work for members and will make the code simpler.
Why do we choose to special-case the members?

This might be addressed after landing the patch, just wanted to get the rationale behind special-casing members.

This revision is now accepted and ready to land.Jun 15 2018, 3:25 AM
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.