This is an archive of the discontinued LLVM Phabricator instance.

[CodeComplete] Complete a lambda when preferred type is a function
ClosedPublic

Authored by ilya-biryukov on May 22 2019, 2:45 AM.

Event Timeline

ilya-biryukov created this revision.May 22 2019, 2:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 22 2019, 2:45 AM
kadircet added inline comments.May 22 2019, 4:37 AM
clang/lib/Sema/SemaCodeComplete.cpp
4135

This looks cheesy, do we really want to perform this operation only for templates with "function" in their names?

As discussed offline maybe perform this for any template with a single(non-defaulted?) argument, or maybe even better perform a type-check as suggested by you. But I believe there would be too many false positives with the current state.

4154

maybe also add a placeholder for captures?

ilya-biryukov marked 3 inline comments as done.
  • Add placeholder for captures.
  • Only accept classes that have exactly one template argument.
ilya-biryukov marked an inline comment as done and an inline comment as not done.May 22 2019, 6:11 AM
ilya-biryukov added inline comments.
clang/lib/Sema/SemaCodeComplete.cpp
4135

WDYT about "function" + only template with a single arg?
FWIW, I think function in template arguments are very rare, so I'm more optimistic about false-positives even in the current form.

We have an option of making it super-restrictive and only firing on whitelisted things like std::function, boost::function, etc.
But we don't have a mechanism to configure those from the outside, so that will probably turn out too restrictive.

4154

Done. With = as a default.
I'd personally rather have less placeholders, but I guess that might be useful.

kadircet added inline comments.May 22 2019, 7:41 AM
clang/lib/Sema/SemaCodeComplete.cpp
4125

I thought you decided to get rid of this branch after the offline discussions, sorry for not mentioning in earlier revision.

4135

I don't think adding "function" as a restriction is helping much on reducing false positives. I would rather get rid of that check to increase usability.

The real problem is rather checking constructors to make sure there is at least one for which we can provide a callable with the signature we found. Anything else just seems cheesy and might result in people adding more and more cases over time.

It is up to you, I am OK if you choose to keep this restriction as well.

4142

what about referencetype?

void test();
void (&y)() = test;
ilya-biryukov marked 4 inline comments as done.
  • Only work on sugared types
  • Do not check for 'function' in the name
clang/lib/Sema/SemaCodeComplete.cpp
4125

Sorry, my bad, forgot about it.

4135

Removed the restriction on the function way.
I don't think there's an easy way to check for conversions, though, I hope the false-positive rate would super-small.
If not, I'll find a way to check conversions or whitelist only known function names.

4142

Lambdas are r-values, so suggesting them for l-value references would lead to an error.
R-value references to functions are so rare that I feel handling those is not worth it.

kadircet accepted this revision.May 22 2019, 8:08 AM
kadircet marked an inline comment as done.

LGTM, thanks!

clang/lib/Sema/SemaCodeComplete.cpp
4142

ah right

This revision is now accepted and ready to land.May 22 2019, 8:08 AM

Thanks for a quick review!

  • Remove a leftover from name check.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2019, 12:46 AM