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

Update EasyIterator to work with C++20 #13

Merged
merged 6 commits into from
Oct 14, 2024

Conversation

pr0g
Copy link
Contributor

@pr0g pr0g commented Oct 13, 2024

Summary

This PR updates the excellent EasyIterator library to work with C++20 (along with a few minor changes to CMakeLists.txt files and some small comment fixes... sorry I couldn't help myself 😅🙈).

Detail

C++20 comes with an enormous overhaul to how comparisons work (there's an excellent blogpost about this by Barry Revzin you can find here).

When using C++20 with EasyIterator (you can do this by changing cxx_std_17 to cxx_std_20 in the root CMakeLists.txt file when using the library or building the tests), compiling EasyIterator as-is unfortunately no longer compiles. This is down to changes relating to lookup rules, and reversibility and rewriting in C++20. To workaround these changes I've moved the equality operators to friend functions which solves the specific issue relating to reversibility, namely...

.../easy_iterator.h:400:24: error: ISO C++20 considers use of overloaded operator '!=' (with operand types 'easy_iterator::Iterator<std::tuple<easy_iterator::RangeIterator<int>, std::__wrap_iter<int *>>, easy_iterator::increment::ByTupleIncrement, easy_iterator::dereference::ByTupleDereference, easy_iterator::compare::ByLastTupleElementMatch>' and 'easy_iterator::Iterator<std::tuple<easy_iterator::RangeIterator<int>, std::__wrap_iter<int *>>, easy_iterator::increment::ByTupleIncrement, easy_iterator::dereference::ByTupleDereference, easy_iterator::compare::ByLastTupleElementMatch>') to be ambiguous despite there being a unique best viable function with non-reversed arguments [-Werror,-Wambiguous-reversed-operator]

.../easy_iterator.cpp:218:5: note: in instantiation of function template specialization 'easy_iterator::copy<easy_iterator::WrappedIterator<easy_iterator::RangeIterator<int>>, std::vector<int>, easy_iterator::dereference::ByValueReference>' requested here
  218 |     copy(range(10), vec);
      |     ^
.../easy_iterator.h:162:38: note: candidate function with non-reversed arguments
  162 |     template <typename... Args> bool operator!=(const IteratorPrototype<Args...> &other) const {
      |                                      ^
.../easy_iterator.h:159:38: note: ambiguous candidate function with reversed arguments
  159 |     template <typename... Args> bool operator==(const IteratorPrototype<Args...> &other) const {

I've run the tests which all pass with the updated changes:

[doctest] doctest version is "2.4.11"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:  12 |  12 passed | 0 failed | 0 skipped
[doctest] assertions: 329 | 329 passed | 0 failed |
[doctest] Status: SUCCESS!

Note

I have also tested these changes with C++17 to ensure the updates don't break any clients who are not yet ready to upgrade to C++20 (you can see the C++17 changes in commit a79b099). One cool thing in C++20 is you can omit the != operator if == is provided as it will be automatically provided for you. I've kept the usage requirements for the library as C++17 even though now it will work with C++20.

If you don't care about C++ 20 no worries, feel free to close this PR, but I'll be using this branch out of my fork for the foreseeable future I'm sure 😅.

I'd also like to quickly say this is a great library and one I've enjoyed using for many years!

Copy link
Owner

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Amazing, thanks a lot for updating the project to remain compatible with C++20! I'm very happy to update the project with your newest changes so that others can benefit from it. In order to merge, I'll need to push an update to the workflow configs so that CI actions will run for third-party PRs.

@pr0g
Copy link
Contributor Author

pr0g commented Oct 14, 2024

Amazing, thanks a lot for updating the project to remain compatible with C++20! I'm very happy to update the project with your newest changes so that others can benefit from it. In order to merge, I'll need to push an update to the workflow configs so that CI actions will run for third-party PRs.

No problem at all! 🙂 Thanks for creating such a great library in the first place, and for approving the PR 🥳

I see the builds have passed which is great, I just see the style check failed. Hopefully that's not too much of a pain to fix (I did run clang-format locally but might have missed something sorry).

Thanks very much for the swift response!

@TheLartians
Copy link
Owner

I see the builds have passed which is great, I just see the style check failed. Hopefully that's not too much of a pain to fix (I did run clang-format locally but might have missed something sorry).

Yeah seems the expected version of clang-format wasn't clearly specified in the CI, sorry about that! Now that it's locked I can format with the same version on my machine and the tests pass. In the future we may also want to run the CI with different versions of C++ to ensure backwards compatibility, but for now supporting C++20 is already a great improvement.

I'll prepare an update in another PR and make a new release shortly. Thanks again for the contribution!

@TheLartians TheLartians merged commit ae5bb8b into TheLartians:master Oct 14, 2024
6 checks passed
@pr0g
Copy link
Contributor Author

pr0g commented Oct 14, 2024

I see the builds have passed which is great, I just see the style check failed. Hopefully that's not too much of a pain to fix (I did run clang-format locally but might have missed something sorry).

Yeah seems the expected version of clang-format wasn't clearly specified in the CI, sorry about that! Now that it's locked I can format with the same version on my machine and the tests pass. In the future we may also want to run the CI with different versions of C++ to ensure backwards compatibility, but for now supporting C++20 is already a great improvement.

I'll prepare an update in another PR and make a new release shortly. Thanks again for the contribution!

No worries at all, thanks for updating it! Okay awesome stuff, thanks for running the formatting again 🙌

In the future we may also want to run the CI with different versions of C++ to ensure backwards compatibility.

100%, it should be possible to create a stub CMake executable in the test project that includes EasyIterator and sets target_compile_features to cxx_std_20 (the library can still have the usage requirement of cxx_std_17), and that would guarantee it compiles with C++20 (we could do the same for C++23 as well perhaps when it's a bit more established).

Thank you very much for merging the PR and responding so quickly, I'll delete the pr0g:cpp20-support now 👍

@pr0g pr0g deleted the cpp20-support branch October 14, 2024 15:20
@TheLartians
Copy link
Owner

Changes are now released in v1.5. 🎉
Thanks again for the update!

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.

2 participants