Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<tuple>: Overhaul tuple_cat() for simplicity and less special-casing #2833

Merged
merged 11 commits into from
Jul 1, 2022

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Jun 29, 2022

Resolves #260. Inspired by @ArtemSarmini's #461, although this takes a different approach. See #461 (comment) for rationale: despite the issue's title, this PR is not generalizing tuple_cat() beyond pair and array at this time (except for ranges::subrange), nor is it using get as a customization point.

This PR can be viewed as a series of transformations (see individual commits, each of which build and pass tests), but it's simpler to explain as an overall before-and-after change:

  • This removes _Cat_sequences, _View_as_tuple, and _Repeat_for. We'll achieve the same results in different ways.
  • tuple_cat():
    • The return type is now a _Ret typedef instead of type, for increased consistency.
    • The tag types are being renamed to reduce verbosity ("arg" was meaningless here).
    • All of the types are being extracted as aliases, so that the return statement fits on a single line and is easier to read.
  • _Tuple_cat():
    • The function argument is forward_as_tuple(), so it's always a modifiable rvalue. Thus we can take _Ty _Arg and pass _STD move(_Arg), there's no need to forward it.
    • For style, construct _Ret with braces.
  • _Tuple_cat1:
    • Is now an alias template, since we don't need a more expensive struct here.
    • Now it passes tuple<_Tuples&&...> to _Tuple_cat2; this is the type of the meta-tuple (what forward_as_tuple() returns). There it will be referred to as _Ty, exactly matching _Ty _Arg.
    • It also feeds make_index_sequence<SIZES>... to _Tuple_cat2, which will work (almost) purely with index sequences. These sequences (patterns like <0, 1, 2>, <0, 1, 2, 3, 4>, <0, 1>, etc.) will be concatenated to form the _Kx sequence. They are also transformed to form the _Ix sequence.
    • The sizes are tuple_size_v<_Remove_cvref_t<_Tuples>>. This is part of how we're replacing _View_as_tuple, by viewing the arguments through the lens of tuple_size_v. The _Tuples are being perfectly forwarded, so we need to remove references (and we may as well remove cv-qualifiers, since the _Remove_cvref_t helper is optimized, even though tuple_size_v would ignore them). We don't need the full power of decay, as arrays and functions aren't interesting here.
    • The insight here is that the _Kx and _Ix indices, combined with the meta-tuple _Ty, are sufficient to determine the return type. This significantly simplifies that machinery.
  • _Tuple_cat2's primary template:
    • Takes _Ty (used at the very end of recursion).
    • class _Kx_arg, class _Ix_arg, size_t _Ix_next have the same meanings as before.
    • Takes _Sequences instead of _Tuples.
    • This "always succeeds" so we don't need a static_assert (non-tuple-like inputs would have been rejected at the tuple_size_v layer).
  • _Tuple_cat2 has a specialization to handle empty _Sequences when recursion is done.
    • As mentioned, we compute _Ret from _Kx, _Ix, and _Ty.
    • The meta-tuple is a modifiable rvalue, so we can directly say tuple_element_t<_Ix, _Ty>.
    • The elements of the meta-tuple will be lvalue references or rvalue references (that's what forward_as_tuple() does), which we need to remove. They can also be cv-qualified (depending on the original _Tuples), and we must remove that, or tuple_element_t will "helpfully" propagate cv-qualifiers. (That is, is_same_v<tuple_element_t<0, const tuple<int>>, const int> is true, but we don't want such propagation here.) However, we don't need the full power of decay_t which we were using earlier.
    • Using tuple_element_t to determine _Ret completes the replacement of _View_as_tuple - as long as tuple_size_v reports the size of a tuple-like "container" and tuple_element_t extracts the element type, this will work.
  • _Tuple_cat2's recursion specialization:
    • Is (hopefully) easier to understand, since it's doing less work than before (no _Ret computation until the end, so it doesn't have to mention tuple a bunch of times), and is directly doing the remaining work.
    • To concatenate the _Kx indices, we can directly say index_sequence<_Kx..., _Kx_next...>.
    • To form the _Ix indices, now that we have a pack of _Kx_next values (instead of types), we can expand directly instead of using _Repeat_for to ignore the types. All we need is an expression that results in _Ix_next repeatedly, and "depends on" _Kx_next to drive the expansion but ignores its values. (_Ix_next + 0 * _Kx_next)... makes this clear, while avoiding any "conditional expression is constant" warnings.

I believe that the result is significantly simpler to understand, and prepares tuple_cat() for possible future developments where tuple_size_v and tuple_element_t handle more types.

Dev11_0000000_tuple_cat provides coverage of different-length tuples, pair and array inputs, reference elements, and even had some coverage of cv-qualification and value categories for the tuple-like objects. However, because the return type logic is the biggest change in this PR, and needed _Remove_cvref_t to handle such scenarios, I am adding extra validation (to ensure that the return type is not influenced by the cv-quals and value categories of the tuple-like objects, and purely concatenates the element types within).

@CaseyCarter noted that this automatically works with ranges::subrange which supports the tuple protocol; I am adding test coverage for that.

StephanTLavavej and others added 9 commits June 29, 2022 05:00
Co-authored-by: ArtemSarmini <16746066+ArtemSarmini@users.noreply.github.com>
Co-authored-by: ArtemSarmini <16746066+ArtemSarmini@users.noreply.github.com>
Co-authored-by: ArtemSarmini <16746066+ArtemSarmini@users.noreply.github.com>
tuple_cat() now sends tuple_size_v to _Tuple_cat1. This captures everything we need to know about the tuple-like types, so we don't need _View_as_tuple anymore.

Then, _Tuple_cat1 sends make_index_sequence<_Nx>... to _Tuple_cat2. These are the individual "K" sequences that need to be concatenated (and transformed to produce the "I" sequences).

Now, _Tuple_cat2 can directly concatenate index_sequence<_Kx..., _Kx_next...>, so we don't need _Cat_sequences anymore.

To repeat _Ix_next, we don't need a _Repeat_for helper, we can just write an expression `(_Ix_next + (0 * _Kx_next))...` that ignores the pack.

Finally, we should rename _Tuple_cat2's primary parameter from _Tuples to _Sequences because that's what it takes now.

Co-authored-by: ArtemSarmini <16746066+ArtemSarmini@users.noreply.github.com>
Co-authored-by: ArtemSarmini <16746066+ArtemSarmini@users.noreply.github.com>
…before _Tuple_cat2.

Co-authored-by: ArtemSarmini <16746066+ArtemSarmini@users.noreply.github.com>
The "arg" in `_Kx_arg_seq` and `_Ix_arg_seq` didn't serve a purpose.

Co-authored-by: ArtemSarmini <16746066+ArtemSarmini@users.noreply.github.com>
…tainers.

Co-authored-by: ArtemSarmini <16746066+ArtemSarmini@users.noreply.github.com>
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Jun 29, 2022
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner June 29, 2022 20:53
@CaseyCarter CaseyCarter self-assigned this Jun 29, 2022
stl/inc/tuple Outdated Show resolved Hide resolved
stl/inc/tuple Outdated Show resolved Hide resolved
stl/inc/tuple Outdated Show resolved Hide resolved
stl/inc/tuple Show resolved Hide resolved
@CaseyCarter CaseyCarter removed their assignment Jun 30, 2022
@StephanTLavavej StephanTLavavej self-assigned this Jun 30, 2022
@StephanTLavavej
Copy link
Member Author

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej
Copy link
Member Author

I've had to push an additional commit to fix an internal compiler test that was evaluating decltype(tuple_cat(declval<tuple<Abstract>>(), blah)). This moves return type computation back into the struct, but otherwise preserves the new technique.

Changelog for the latest commit:

  • tuple_cat() no longer uses auto. Instead it gets _Ret from _Tuple_cat1 and passes it to _Tuple_cat(). (This was type in main, but I think consistently using _Ret is clearer.)
  • _Tuple_cat1 needs to take _Tuples... now; the previous tuple_size_v<_Remove_cvref_t<_Tuples>>... is being moved into it.
  • _Tuple_cat() no longer computes _Ret and returns auto. Instead it takes and uses _Ret. The _Ret computation is moving into _Tuple_cat2.
  • _Tuple_cat1 additionally forms the type of the meta-tuple (that's tuple<_Tuples&&...>, the return type of forward_as_tuple()), and passes it to _Tuple_cat2. Note that we will use _Ty for this, exactly matching _Ty _Arg.
  • _Tuple_cat2's recursion passes _Ty down, not using it until the end.
  • Now, we have a separate primary template (no definition) and specialization to handle when the _Sequences are gone (thus, no internal static assert). This specialization exists to inspect the _Kx... and _Ix... packs, because we need to form _Ret.

Ultimately, this results in a solution that is almost as elegant, except that it has to pass the meta-tuple's type into the struct machinery. It makes the (weird) abstract class scenario work, by never theoretically constructing any tuple<Abstract> objects.

I am intentionally not adding coverage for that scenario here, as the compiler suite provides coverage, and it appears to be highly sensitive to compiler bugs (the test was originally added due to a compiler regression, and it is still sensitive to std:: versus using namespace std; which should make no difference; we have enough compiler bugs to worry about).

stl/inc/tuple Show resolved Hide resolved
@StephanTLavavej StephanTLavavej merged commit c34f249 into microsoft:main Jul 1, 2022
@StephanTLavavej StephanTLavavej deleted the tuple-cat-2022 branch July 1, 2022 02:31
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
…ing (microsoft#2833)

Co-authored-by: ArtemSarmini <16746066+ArtemSarmini@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<tuple>: tuple_cat() should be generalized beyond pair and array
3 participants