Page MenuHomePhabricator

Force is_invocable template parameters to be complete types
AbandonedPublic

Authored by zoecarver on Apr 7 2019, 8:07 PM.

Details

Summary

This patch adds __complete_type (along with some helpers). It also updates is_invocable to assert that it's template parameters are complete types, void, or an unbounded array.

The patch introduces a lot of helper structs which create some bloat. If you decide that it would be better if it were more condensed, that is fine with me. I decided to start with maximum verbosity though.

Diff Detail

Event Timeline

zoecarver created this revision.Apr 7 2019, 8:07 PM
zoecarver marked 3 inline comments as done.Apr 7 2019, 8:12 PM
zoecarver added inline comments.
include/__tree
973

Adding a check using __complete_type might help fix this issue (41360).

include/type_traits
4451

Is there a better way to include function types?

4504

I am not sure if it is better to static_assert this or to make it affect value.

EricWF requested changes to this revision.Apr 9 2019, 9:38 PM

This patch isn't correct. The is_flexable_invocable trait is an ODR violation. And I don't think the bug this is solution to PR41360.

include/type_traits
4437

This is an ODR violation in waiting.

include/utility
1596

The types have to be complete here. It's an ODR violation otherwise.

This revision now requires changes to proceed.Apr 9 2019, 9:38 PM

Your right, PR41360 is only a wording issue. I also agree __flexible_invokable is bad, but I don't see another way to check invokability while allowing incomplete types (which I think is necessary for containers).

As for __invokable_r, I think the two options are either ignore the UB or implement some version of __complete_type. I was just going off what the comment said needed to be done.

Your right, PR41360 is only a wording issue. I also agree __flexible_invokable is bad, but I don't see another way to check invokability while allowing incomplete types (which I think is necessary for containers).

I'm not sure it's necessary, but the way libstdc++ addressed this seems preferable. They moved the triggering of the diagnostic into the destructor. Which meant the types couldn't be incomplete when it was evaluated.
This used to be what we did, but @ldionne reverted it in r348529 to improve the diagnostic message. I think we should revert that commit.

As for __invokable_r, I think the two options are either ignore the UB or implement some version of __complete_type. I was just going off what the comment said needed to be done.

We had a __check_complete implementation previously. I removed it because it's near impossible to correctly check that *all* the types are complete in a call expression. See llvm.org/PR37407 and r332040.

Your right, PR41360 is only a wording issue. I also agree __flexible_invokable is bad, but I don't see another way to check invokability while allowing incomplete types (which I think is necessary for containers).

I'm not sure it's necessary, but the way libstdc++ addressed this seems preferable. They moved the triggering of the diagnostic into the destructor. Which meant the types couldn't be incomplete when it was evaluated.
This used to be what we did, but @ldionne reverted it in r348529 to improve the diagnostic message. I think we should revert that commit.

I think we should move my check back to the destructor, yes. However, reverting the commit as a whole results in worse error messages. You can retain slightly better error messages AND avoid crazy forward declarations AND fix the issue discussed in PR41360. I'll submit a patch.

Thanks @ldionne. Let me know if you want me to help at all.

zoecarver marked an inline comment as done.Apr 10 2019, 9:00 PM
zoecarver added inline comments.
include/type_traits
4419

This is now the only issue we still have to resolve. The standard says all these types must "be a complete type, (possibly cv-qualified) void, or an array of unknown bound. Otherwise, the behavior is undefined." Which (I think) means that we should either put an error/warning here or just let it be UB and get rid of the comment.

EricWF added inline comments.Apr 10 2019, 10:02 PM
include/type_traits
4419

We should keep the comment or replace it with a bug, but implementing the naive library implementation isn't what we want.

Also, it's worth noting checking for completeness doesn't address PR41360. Foo* is a complete type even if Foo isn't. For example, the return type, function type, and argument types are all complete in the following code:

struct Base {};
struct Derived;
void sink(Base*);
void test(Derived* p) { std::invoke(&sink, p); }
struct Derived : Base {};

And there are many variations of this example, such as:

namespace NS { struct X; }
void baz(...) = delete;
void test2(const NS::X &x) { baz(x); }
struct NS::X { friend void baz(const X&); };

Additionally, the current wording is grossly underconstrained. It allows invoke_result_t<Func, Incomplete&&>, but forbids invoke_result_t<Func, Incomplete> even though they're functionally equivalent (declval<T>() and declval<T&&>() return the same type).

Also the cost if instantiating all the templates required to check for completeness is non-trivial.

Perhaps we should open a bug about this, file a standard issue, and see if we can add a builtin type-trait to clang which handles this checking for us.

zoecarver marked an inline comment as done.Apr 11 2019, 8:04 AM
zoecarver added inline comments.
include/type_traits
4419

Also, it's worth noting checking for completeness doesn't address PR41360. Foo* is a complete type even if Foo isn't. For example, the return type, function type, and argument types are all complete in the following code:

Do you mean that it should not be treated as a complete type or that it is not being treated as a complete type?

Anyway, I completely agree with your last point. How hard do you expect it would be to add a builtin? I have been looking for a relatively easy one to start with.

zoecarver abandoned this revision.Feb 24 2020, 10:16 AM