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>: tuple_cat for tuple-like types #461

Closed
wants to merge 3 commits into from

Conversation

ArtemSarmini
Copy link
Contributor

Description

Closes #260. I also replaced recursive impl of _View_as_tuple for arrays with index_sequence.

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

  • Identifiers in product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
  • The STL builds successfully and all tests have passed (must be manually
    verified by an STL maintainer before automated testing is enabled on GitHub,
    leave this unchecked for initial submission).
  • These changes introduce no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate
    or trivially copyable, etc.).
  • These changes were written from scratch using only this repository,
    the C++ Working Draft (including any cited standards), other WG21 papers
    (excluding reference implementations outside of proposed standard wording),
    and LWG issues as reference material. If they were derived from a project
    that's already listed in NOTICE.txt, that's fine, but please mention it.
    If they were derived from any other project (including Boost and libc++,
    which are not yet listed in NOTICE.txt), you must mention it here,
    so we can determine whether the license is compatible and what else needs
    to be done.

@ArtemSarmini ArtemSarmini requested a review from a team as a code owner January 26, 2020 15:29
@ArtemSarmini

This comment was marked as outdated.

stl/inc/tuple Outdated Show resolved Hide resolved
@ArtemSarmini

This comment was marked as outdated.

@azure-pipelines

This comment was marked as outdated.

@ArtemSarmini

This comment was marked as outdated.

@StephanTLavavej

This comment was marked as outdated.

@StephanTLavavej

This comment was marked as outdated.

@azure-pipelines

This comment was marked as outdated.

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually implement #260 since we use _STD get<_Kx> on line 951 which only supports array, pair, and tuple. I think we need to arrange for that to be the qualified std::get for standard types and unqualified get for non-standard types.

stl/inc/tuple Outdated Show resolved Hide resolved
stl/inc/tuple Outdated Show resolved Hide resolved
stl/inc/tuple Show resolved Hide resolved
stl/inc/tuple Outdated Show resolved Hide resolved
stl/inc/tuple Outdated Show resolved Hide resolved
stl/inc/tuple Outdated Show resolved Hide resolved
@ArtemSarmini

This comment was marked as resolved.

@ArtemSarmini

This comment was marked as resolved.

@CaseyCarter

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Feb 5, 2020
@CaseyCarter

This comment was marked as resolved.

@BillyONeal
Copy link
Member

Hi there, @ArtemSarmini are you still working on this?

@cbezault

This comment was marked as outdated.

@azure-pipelines

This comment was marked as outdated.

@cbezault cbezault closed this Oct 7, 2020
@cbezault cbezault reopened this Oct 7, 2020
@cbezault

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@cbezault

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@cbezault

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

Base automatically changed from master to main January 28, 2021 00:35
@CaseyCarter CaseyCarter self-requested a review April 28, 2021 21:35
@CaseyCarter CaseyCarter self-assigned this Apr 28, 2021
@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 461 in repo microsoft/STL

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@StephanTLavavej
Copy link
Member

It looks like @ArtemSarmini hasn't had time to look into this for the last couple of years - totally understandable, since the maintainer team was also incredibly busy! 😹 I'd like to ensure that the original contributor gets credit for looking into this issue, even if they've moved on to other things, so I'll pick up this PR (which is now our oldest active PR and the only triple-digit one). I believe the main issues are:

  • Test coverage is needed (as the PR was created before we added the test harness, I believe),
  • When I filed <tuple>: tuple_cat() should be generalized beyond pair and array #260 (from my personal todo list of "stuff that could be improved"), I wasn't thinking too hard about the consequences of enlarging our support for a non-Standard extension - which is something that we generally try to avoid these days. I was mostly thinking about simplifying our existing support for pair and array.
  • There is now a proposal being reviewed by Library Evolution, P2165R3 "Compatibility between tuple, pair and tuple-like objects", that is highly relevant. In the latest revision R3, it has reduced its scope to types with std::get. Although this is not part of the Working Draft, I think this is a good reason to limit any rework here to only using std::get, instead of trying to accept get as a customization point far in advance of the Standard coming up with a design in this area.

Thus I believe that different code changes are needed, and will force-push this PR to start from today's main.

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 461 in repo microsoft/STL

@StephanTLavavej
Copy link
Member

Actually, the Azure Pipelines bot is acting weird here; I will instead create a fresh PR and use Co-authored-by for crediting.

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
5 participants