This is an archive of the discontinued LLVM Phabricator instance.

[ADT] function_ref's constructor is unavailable if the argument is not callable.
ClosedPublic

Authored by sammccall on Oct 6 2020, 7:37 AM.

Details

Summary

This allows overload sets containing function_ref arguments to work correctly
Otherwise they're ambiguous as anything "could be" converted to a function_ref.

This matches proposed std::function_ref, absl::function_ref, etc.

Diff Detail

Event Timeline

sammccall created this revision.Oct 6 2020, 7:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2020, 7:37 AM
sammccall requested review of this revision.Oct 6 2020, 7:37 AM
kadircet accepted this revision.Oct 6 2020, 9:41 AM

thanks, LGTM!

This revision is now accepted and ready to land.Oct 6 2020, 9:41 AM
dblaikie added inline comments.
llvm/include/llvm/ADT/STLExtras.h
199–209

This now has two different SFINAE approaches (one in a defaulted template type parameter, one in a defaulted function/ctor parameter) - do they need to be different? Could they be merged in some way to at least use the same/common infrastructure?

kadircet added inline comments.Oct 6 2020, 12:20 PM
llvm/include/llvm/ADT/STLExtras.h
199–209

i believe this is to prevent creation of function_ref from function_ref, so it is different than what the template type parameter is doing. but I suppose they can be merged.

dblaikie added inline comments.Oct 6 2020, 12:26 PM
llvm/include/llvm/ADT/STLExtras.h
199–209

Yep, I understand that there are two different things that need to be tested - but if they don't need to be tested in different ways, it'd be good if they were merged to reduce confusion when reading the code (like seeing two variables next to each other with different naming conventions - raises questions about whether the differences are important/intentional/significant in some way the reader doesn't understand, etc).

sammccall added inline comments.Oct 7 2020, 6:06 AM
llvm/include/llvm/ADT/STLExtras.h
199–209

Good point, I merged them to use the existing default-param technique.

Because the compatible-types condition is complex, I extracted it to a variable template.

sammccall updated this revision to Diff 296670.Oct 7 2020, 7:30 AM

Combine SFINAE conditions into one top-level condition.

Fix a couple of broken usages that no longer compile (in default-args that were
never used, so template instantiation didn't catch them).

sammccall updated this revision to Diff 296671.Oct 7 2020, 7:30 AM

Clang-format

This revision was landed with ongoing or failed builds.Oct 7 2020, 7:31 AM
This revision was automatically updated to reflect the committed changes.
Harbormaster completed remote builds in B74271: Diff 296670.