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

C++03/move compliance, better support for compiler/library feature detection #22

Merged
merged 6 commits into from
May 22, 2017
Merged

C++03/move compliance, better support for compiler/library feature detection #22

merged 6 commits into from
May 22, 2017

Conversation

mattreecebentley
Copy link
Contributor

@mattreecebentley mattreecebentley commented May 22, 2017

Included <functional> (technically required for std::less)

@gfx gfx merged commit e39ca95 into timsort:master May 22, 2017
@gfx
Copy link
Member

gfx commented May 22, 2017

Looks good. Thanks.

@mattreecebentley
Copy link
Contributor Author

You're welcome!

@Morwenn
Copy link
Member

Morwenn commented Sep 22, 2019

I was rewriting the tests, and it turns out that this change doesn't work with move-only types (and we have such types in the tests). This is because the ternary expression requires the type to have a copy operation, even when the move operation is picked instead because of template instantiation rules.

I don't understand the original rationale for this change: move-only types are a known pattern, but I don't think that copy-only types serve a purpose. As far as I know when only the copy operation is specified, then the compiler generates a move operation equivalent to the copy one (for value types, move is merely an optimization). The standard library itself doesn't try to support types that have copy operations and deleted move operations. I feel like we shouldn't need to support those types either, especially if they break move-only types, which are arguably more common.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants