Conversation
|
I made the following design decisions while implementing this PR. Happy to discuss/change any of them as appropriate. The tests cover these edge cases should we want to modify behavior.
I tried to keep the architecture mostly intact, but refactored some things to avoid passing a million distinct booleans around everywhere. Any feedback is much appreciated! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #356 +/- ##
==========================================
+ Coverage 74.06% 74.46% +0.39%
==========================================
Files 9 9
Lines 860 881 +21
Branches 214 216 +2
==========================================
+ Hits 637 656 +19
Misses 24 24
- Partials 199 201 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
philipcraig
left a comment
philipcraig
left a comment
There was a problem hiding this comment.
Ran the tests and a few probes on GCC trunk. Inline comments cover the conformance rules, the trampolines and the thunk layout. Three things sit outside the diff:
- Stale comments:
protocol_interface_function_infos(line 236) still says ref-qualified members are rejected and points at a TODO onmethod_thunkthat this PR deletes;is_protocol_conformant(line 802) still explains why it calls the function instead of reading the variable in terms of a throw that no longer happens;make_view_vtable(line 739) still saysall_constpopulates every entry. conformance_check_rejectsin protocol_test.cc (line 407) has no callers left and its comment describes the old rejection. Deleting the#ifdef __cpp_constexpr_exceptionsblock with it. A positive test for the deleted static case would be worth keeping:struct I { int take() &&; }; struct C { static int take(); };conforms and dispatches throughprotocol<I>.- Docs: the Limitations bullet in CONTRIBUTING.md (line 190) still says the generated wrapper does not apply the qualifier, and DRAFT.md says nothing about value-category propagation or about views omitting
&&members, which #97 asked the draft to acknowledge.
| // parameter types; noexcept is not compared. | ||
| consteval bool same_signature(std::meta::info candidate, | ||
| std::meta::info interface) { | ||
| if (is_rvalue_reference_qualified(interface) != |
There was a problem hiding this comment.
Only the && qualifier is compared, so & is never checked in either direction and #268 is answered implicitly. With struct A { int foo() & { return 2; } int foo() && { return 1; } }; against interface int foo(), std::move(p).foo() returns 2 while std::move(a).foo() returns 1. A &-only candidate also conforms to int foo() even though moving the concrete object is ill-formed, so the protocol is more permissive than the type. The comment above says "cvref qualifiers" and the test is named RefQualifiersMatchExactly, but & is not part of the rule.
Either pick one of the #268 options here (reject &-only candidates for unqualified members, or two vtable entries) and say so in this comment, the member_policy::propagate comment and DRAFT.md, or reword the comment and test name to state the rule as implemented.
| if (is_const(interface) != is_const(remove_reference(obj_type))) return false; | ||
|
|
||
| if (is_rvalue_reference_qualified(interface) != | ||
| is_rvalue_reference_type(obj_type)) |
There was a problem hiding this comment.
The other direction is stricter than it needs to be. For interface int take() &&, an unqualified candidate int take() is rejected here and at line 148, and a by-value explicit-object int take(this C self) is rejected too, while the static branch accepts static int take(). Any ordinary type, including std types, therefore cannot satisfy an && interface even though the trampoline's std::move(*ptr).take() would dispatch correctly. A callability-based rule (the candidate conforms if it is callable in every value category the interface member admits) fixes this and the & case above together.
| find_conforming_member<member, ^^U>(); | ||
| result.[:vtable_member:] = &mutable_view_trampoline<FnPtrType, U, | ||
|
|
||
| // View vtables cannot use rvalue member functions. |
There was a problem hiding this comment.
The drop is silent. is_valid_view_interface still accepts an interface whose only members are &&-qualified, so struct I { int f() &&; }; protocol_view<I> v(c); compiles and v has no member f at all; the first diagnostic is "no member named f" at the call site. Either reject names that would vanish under all_const/const_only in is_valid_view_interface or generate_wrapper_bases, or state the rule on protocol_view and in DRAFT.md.
| struct wrapper_bases : MemberBases... {}; | ||
|
|
||
| // Returns the appropriate const, noexcept, and reference qualification for the | ||
| // member based on the const policy. |
There was a problem hiding this comment.
"const policy" is the old name; this now takes a member_policy.
| // viewed/owned object. | ||
| R operator()(Args... args) noexcept(IsNoexcept) | ||
| requires(!IsConst) | ||
| R operator()(Args... args) & noexcept(is_noexcept) |
There was a problem hiding this comment.
The four operator() bodies differ only in the const on the enclosing() cast, so the valueless assert, the vtable load and the splice call now have to change in four places. A private static
template <typename P>
static R call(P* protocol_object, Args&&... args) noexcept(is_noexcept);holding the body, with each qualified overload a one-line return call(static_cast<ProtocolType*>(enclosing(this)), std::forward<Args>(args)...);, passes the PR's test file and removes about thirty lines. A single deducing-this operator() is not a drop-in: under all_const, p.foo() becomes ambiguous between the & and const& thunks brought in by member_thunk's using-declarations.
| std::string(name) + "'"); | ||
| } | ||
|
|
||
| enum class member_options { |
There was a problem hiding this comment.
Two of these flags are derivable from Member (is_noexcept, and is_const under propagate), and encoding "unqualified" as is_lvalue | is_rvalue means every unqualified member and every view member now gets two constrained ref-qualified overloads where one unqualified or const operator() did before. On GCC trunk the unchanged pre-PR test corpus compiles about 10% slower with the new header, and a 40-member TU calling members in both value categories emits 180 thunk bodies instead of 100 at -O0 (identical at -O2). The static members is_noexcept/is_const also shadow std::meta::is_noexcept/is_const inside method_thunk.
Passing overload_spec<Member, member_policy> and deriving the booleans inside method_thunk, with a three-state ref qualifier (none, &, &&), keeps the single operator() for unqualified and view members. GCC trunk accepts mutually exclusive constrained unqualified and ref-qualified overloads in one class, so the thunk does not need splitting.
|
Thanks for listing the decisions, that made them easy to check. Three notes beyond the inline review. Decision 1 and #268. On #268 @jbcoe preferred One rule for all five. The decisions fall out of a single statement: a candidate conforms if it is callable in every value category the interface member admits. That gives: unqualified interface members accept unqualified, Legal overload sets. [over.load]/2.3 forbids mixing ref-qualified and unqualified overloads of one name, so once an interface has On views I agree with not propagating, though for a different reason than the const precedent: a view is a handle, and moving the handle says nothing about the viewed object, as with a moved |
|
If we did want r-value-qualified member functions called from views, we probably want |
Co-authored-by: Philip Craig <689193+philipcraig@users.noreply.github.com>
Co-authored-by: Philip Craig <689193+philipcraig@users.noreply.github.com>
|
@jbcoe I think this has probably fallen far enough behind that it's worth doing over, but curious what you think. |
|
If you'd like to pair-program a painful manual rebase over coffee it could be instructive, or character building. |
That would certainly wake me up in the morning. Any preferred times? |
|
@RyanJK5 I'm sorry that we never got to rebase this face-to-face. Would you like me to rebase this PR on the plane? |
No worries. If you're up for it, that would be really helpful. |
3 participants
Closes #97 and #268