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

Fix concat_type_lists_unique_t to be "more unique" #546

Merged
merged 6 commits into from
Jun 27, 2023

Conversation

ccotter
Copy link
Contributor

@ccotter ccotter commented Jun 26, 2023

The current behavior of concat_type_lists_unique_t is a little confusing.

concat_type_lists_unique_t<type_list<int, int>, type_list<int>> ->
    type_list<int, int>
concat_type_lists_unique_t<type_list<int>, type_list<int, int>> ->
    type_list<int>

In other words, concat_type_lists_unique_t is not commutative.

The docs seemed to indicate concat_type_lists_unique_t expected the lists to already contain unique elements (the comment seemed incomplete though). I am changing the behavior of concat_type_lists_unique_t to remove duplicate elements from the input lists, since it conveniently fixes any sender algos that use it to construct the value_types of the output sender to not contain duplicate types.

The current behavior of concat_type_lists_unique_t is a little
confusing.

```
concat_type_lists_unique_t<type_list<int, int>, type_list<int>> ->
    type_list<int, int>
concat_type_lists_unique_t<type_list<int>, type_list<int, int>> ->
    type_list<int>
```

In other words, concat_type_lists_unique_t is not commutative.

The docs seemed to indicate concat_type_lists_unique_t expected the
lists to already contain unique elements (the comment seemed incomplete
though). I am changing the behavior of concat_type_lists_unique_t
to remove duplicate elements from the input lists,  since it
conveniently fixes any sender algos that use it to construct the
value_types of the output sender to not contain duplicate types.
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 26, 2023
});
static_assert(std::is_same_v<
decltype(s)::value_types<std::variant, std::tuple>,
std::variant<std::tuple<double>, std::tuple<int>>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My main goal of this PR was to fix the output sender from upon_error to not contain duplicate values in value_types. I chose to change the behavior of concat_type_lists_unique_t, but I'm happy to take another approach (such as directly changing upon_error and possibly other algos that might have a similar problem).

>);
static_assert(std::is_same_v<
unique_type_list_elements_t<type_list<int, double, int>>,
type_list<double, int>
Copy link
Contributor Author

@ccotter ccotter Jun 26, 2023

Choose a reason for hiding this comment

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

Note that my hasty implementation of unique_type_list_elements_t does not preserve order, or rather, it removes duplicate items from the front rather than end of list.

Copy link
Contributor

@ispeters ispeters left a comment

Choose a reason for hiding this comment

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

Mostly looks good; just a couple of nits, really.

include/unifex/type_list.hpp Show resolved Hide resolved
include/unifex/type_list.hpp Outdated Show resolved Hide resolved
test/type_list_test.cpp Show resolved Hide resolved
Copy link
Contributor

@ispeters ispeters left a comment

Choose a reason for hiding this comment

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

Nice!

@ispeters ispeters merged commit 5668f26 into facebookexperimental:main Jun 27, 2023
@ccotter ccotter deleted the type-list branch June 27, 2023 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants