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

Add support for C++ 20 ranges. #1050

Merged
merged 1 commit into from Jul 21, 2020
Merged

Conversation

llllllllll
Copy link
Contributor

@llllllllll llllllllll commented Jul 20, 2020

C++ 20 adds a new feature called "ranges", which provides components for dealing
with sequences of values: https://en.cppreference.com/w/cpp/ranges.

A range is like a normal object containing begin and end, except there are
also composable operations like maps, filters, joins, etc.
The iterator objects returned by a range's begin and end require a more
strict set of operations than is needed for a range-for loop.

This PR adds the extra operations needed to support turning dom::array and
dom::object into a range.
This PR does not depend on any C++ 20 behavior, the added operators are all
valid C++ 11, and are already part of the LegacyIterator concepts.
This PR adds extra code behind: #if defined(__cpp_lib_ranges) guards, which is
the new C++ 20 specified feature test macro for ranges support. When ranges
support is detected, extra compile time checks are added to ensure that
dom::array and dom::object satisfy the range concept. No runtime tests have
been added yet because these compile time checks should be sufficient.

If desired, the static_assert code could be moved out of the actual code
headers and put into a test file.

EDIT:

This is related to #1046, I apologize for not commenting on that issue before opening this, I checked for an issue about this a few days ago and must have missed this. Using this PR, you could accomplish the keys/values iterator using std::ranges' or ranges-v3's range::view::transform and a lambda. If key_value_pair were made to be tuple like, meaning the type used case 2 from https://en.cppreference.com/w/cpp/language/structured_binding (it is currently case 3), then ranges::views::keys and ranges::views::values could be used directly.

@lemire
Copy link
Member

lemire commented Jul 20, 2020

At a glance, this sounds great...

@TkTech
Copy link
Member

TkTech commented Jul 20, 2020

@llllllllll I saw your use of ranges in your bindings, and it made some elegant code. Definitely a fan of adding what's missing to simdjson. I'm trying to avoid it in pysimdjson however because even ranges-v3 requires at least C++14, and is only tested against VC2019. It would seem to deny Python bindings for Windows to all but Py3.8+, and possibly require compiling a newer GCC into manylinux for Linux wheels.

@llllllllll
Copy link
Contributor Author

I see, if you need to target older compilers then rolling your own iterators will be easier. These changes should still make that work easier though.

Regarding the test failures due to performance regressions, i can't see how my changes would be causing differences in code generation. Do these look like real differences, or are these noisy?

Copy link
Member

@jkeiser jkeiser 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 excited to have range support. I was disappointed that iterators didn't give me easy map / filter syntax like I'm used to in other languages.

This looks great, too!

@jkeiser
Copy link
Member

jkeiser commented Jul 20, 2020

There is absolutely zero reason this should impact performance tests. These objects aren't invoked during the benchmarks we use (which just test parsing speed). I'll re-kick the ones that failed just to double check that.

@lemire
Copy link
Member

lemire commented Jul 20, 2020

@llllllllll @jkeiser I think we need to extend our testing to cover C++20. I know that @llllllllll probably compiled it for C++20 using their compiler, but we need CI tests.

I'd hate for a user to turn on C++20 and find that the code won't build. We need to trust the PR but also verify it.

@lemire
Copy link
Member

lemire commented Jul 20, 2020

@jkeiser I am starting to hate circle ci for performance tests. I just merged new tests into our main branch where we use github actions for performance testing, so we could drop performance testing on circle ci.

@llllllllll Can you sync your PR with master... this should bring in the new GitHub actions.

C++ 20 adds a new feature called "ranges", which provides components for dealing
with sequences of values: https://en.cppreference.com/w/cpp/ranges.

A range is like a normal object containing `begin` and `end`, except there are
also composable operations like maps, filters, joins, etc.
The iterator objects returned by a range's `begin` and `end` require a more
strict set of operations than is needed for a range-for loop.

This PR adds the extra operations needed to support turning `dom::array` and
`dom::object` into a range.
This PR does not depend on any C++ 20 behavior, the added operators are all
valid C++ 11, and are already part of the LegacyIterator concepts.
This PR adds extra code behind: `#if defined(__cpp_lib_ranges)` guards, which is
the new C++ 20 specified feature test macro for ranges support. When ranges
support is detected, extra compile time checks are added to ensure that
`dom::array` and `dom::object` satisfy the range concept. No runtime tests have
been added yet because these compile time checks should be sufficient.

If desired, the `static_assert` code could be moved out of the actual code
headers and put into a test file.
@llllllllll
Copy link
Contributor Author

I agree, C++ 20 should make it into the test matrix for these static_asserts to be checked. There is also some new keywords to check for.

@lemire
Copy link
Member

lemire commented Jul 20, 2020

@llllllllll Which compiler are you using currently (name and version)?

@llllllllll
Copy link
Contributor Author

gcc (GCC) 10.1.0

@lemire
Copy link
Member

lemire commented Jul 20, 2020

I added tests to #1053

@lemire
Copy link
Member

lemire commented Jul 21, 2020

This was approved by @jkeiser and we have tests now for C++20, so let us go!

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.

None yet

4 participants