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 to C++20 #87

Open
6 of 9 tasks
tcbrindle opened this issue Mar 10, 2020 · 11 comments
Open
6 of 9 tasks

Update to C++20 #87

tcbrindle opened this issue Mar 10, 2020 · 11 comments

Comments

@tcbrindle
Copy link
Owner

tcbrindle commented Mar 10, 2020

Now that C++20 has been finalised, NanoRange should aim to match the spec as closely as possible.

Current task list:

tcbrindle added a commit that referenced this issue May 3, 2020
@bstaletic
Copy link
Contributor

Add the algorithms that weren't in P0896 (sample, shift_left/right, any others?)

Those three don't seem to have been "rangified".

Update algorithm result types (again)

Here's a complete list:

  • count and count_if for the range overload should return range_difference_t<R>, not iter_difference_t<iterator_t<R>>.

  • max and min for the range overload should return range_value_t<R>, not iter_value_t<iterator_t<R>>.

  • minmax for the range overload should return minmax_result<range_value_t<R>>, not minmax_result<iter_value_t<iterator_t<R>>>.

  • minmax_element lacks an alias like the following

      template<class I>
      using minmax_element_result = minmax_result<I>;
    
  • partial_sort_copy now has a partial_sort_copy_result return type, which is an alias for copy_result.

  • partition, unique and stable_partition now returns a subrange<I> and borrowed_subrange_t<R>.

count, count_if, min/max "family" and partial_sort_copy were straight-forward to change.

unique, partition and stable_partition were more intrusive and even broke the tests.

Here is my incomplete patch. The tests are were patched in the dirtiest way possible to make them works again. However, I do get an error I can't make sense of. The gist contains the error too.

@tcbrindle
Copy link
Owner Author

Hi, thanks for the patch.

Add the algorithms that weren't in P0896 (sample, shift_left/right, any others?)

Those three don't seem to have been "rangified".

sample has (see here), but it seems the other two have not -- strange, I seem to recall there was a national body comment about this in Belfast, but never mind.

Regarding changing the result types, I was intending to add these types and making copy_result et al aliases for those. This can of course be done after your changes.

Would it be possible to submit your patch as a Github pull request rather than a diff, to allow a CI run? Different compilers might make it easier to diagnose the strange subrange errors.

Thanks again.

@bstaletic
Copy link
Contributor

Regarding changing the result types, I was intending to add these types and making copy_result et al aliases for those. This can of course be done after your changes.

I was going after what's in cppreference, because I still don't know how to navigate eel.is efficiently. That's why I missed things.

Would it be possible to submit your patch as a Github pull request rather than a diff, to allow a CI run? Different compilers might make it easier to diagnose the strange subrange errors.

Absolutely, just trying to clean some stuff up first. I don't like submitting pull requests that don't compile.

@tcbrindle
Copy link
Owner Author

P1243 is the paper adding extra range algorithms for C++20, thanks @al-mission-2016 for the heads-up.

@bstaletic
Copy link
Contributor

For clamp, for_each_n and sample, I have an implementation, but I have not written any tests.

https://gist.github.com/bstaletic/16f1118be9668584d9081ec9c25c5deb

@al-mission-2016
Copy link

Oh, bstaletic, go ahead, please. I'm glad the work is going to be done.

@bstaletic
Copy link
Contributor

@al-mission-2016 Thanks for the heads up. I incorporated the changes that @tcbrindle asked for in #95, but I still have not written the tests.

Here's the branch: https://github.com/bstaletic/NanoRange/tree/new_algos

@tcbrindle
Copy link
Owner Author

@bstaletic Excellent!

For the tests, it's probably easiest to adapt CMCSTL2's sample.cpp (just overwrite the existing, outdated sample.cpp in NanoRange) and Range-V3's for_each_n.cpp, as I have done for most of the rest of NanoRange's tests. Unfortunately neither CMCSTL2 or Range-V3 seem to have clamp() yet, but libc++ has some tests here which you might want to use.

@bstaletic
Copy link
Contributor

I've added the tests and submitted the pull request. #96

@bstaletic
Copy link
Contributor

Regarding [algorithms.results], should the change be as minimal as possible (for example, only change copy_result to alias in_out_result) or more like this where every foo_result is an alias to something in [algorithms.results]?

@tcbrindle
Copy link
Owner Author

The second one. Feel free to submit that PR once #96 is merged.

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

No branches or pull requests

3 participants