Skip to content
New issue

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

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

Already on GitHub? # to your account

use metal::less as default ordering for metal::sort #101

Merged

Conversation

ecrypa
Copy link
Contributor

@ecrypa ecrypa commented Oct 7, 2019

Copy link
Owner

@brunocodutra brunocodutra left a comment

Choose a reason for hiding this comment

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

I'm happy to merge this once unit tests and docs have been updated.

include/metal/list/sort.hpp Outdated Show resolved Hide resolved
@ecrypa ecrypa force-pushed the feature/default-comp-for-sort branch from 25b406f to ffcdd58 Compare October 10, 2019 22:41
test/unit/src/metal/list/sort.cpp Show resolved Hide resolved
include/metal/list/sort.hpp Outdated Show resolved Hide resolved
include/metal/list/sort.hpp Outdated Show resolved Hide resolved
Copy link
Owner

@brunocodutra brunocodutra left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests and docs, to answer your question I think we definitely want this feature.
The only remaining thing is making sure docs read nicely and rebasing on the new convention to prefer class over typename.

include/metal/list/sort.hpp Outdated Show resolved Hide resolved
include/metal/list/sort.hpp Outdated Show resolved Hide resolved
test/unit/src/metal/list/sort.cpp Show resolved Hide resolved
test/unit/src/metal/list/sort.cpp Outdated Show resolved Hide resolved
@ecrypa ecrypa force-pushed the feature/default-comp-for-sort branch 2 times, most recently from 111426c to 3c0c2af Compare October 12, 2019 16:44
Copy link
Owner

@brunocodutra brunocodutra left a comment

Choose a reason for hiding this comment

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

What can we do now?

Worst case I think we should define it in terms of a variadic pack only under METAL_WORKAROUND, which means only MSVC and really old GCC would see a variadic pack, in particular docs would show the right thing.

Having a slightly different behavior for broken compilers is still better than either not having the feature at all or exposing the variadic pack on all compilers.

include/metal/list/sort.hpp Show resolved Hide resolved
using sort = detail::call<
detail::_sort<lbd>::template type,
detail::_sort<lbd...>::template type,
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mind if I open a couple of PRs in order to test different versions? Or should I prefer godbolt.org in order to reduce the load of your CI setup?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't mind extra PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but I think it is not worth any more effort. I vote for the workaround.

@ecrypa ecrypa force-pushed the feature/default-comp-for-sort branch from 06680ac to 2eaf8c9 Compare October 20, 2019 15:42
@ecrypa ecrypa marked this pull request as ready for review October 20, 2019 15:43
@ecrypa ecrypa requested a review from brunocodutra October 20, 2019 17:19
Copy link
Owner

@brunocodutra brunocodutra left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@brunocodutra brunocodutra merged commit 1cd7d07 into brunocodutra:master Oct 24, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants