Add support for C++ 20 ranges. - #1050
Conversation
|
At a glance, this sounds great... |
|
@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. |
|
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? |
jkeiser
left a comment
There was a problem hiding this comment.
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!
|
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. |
|
@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. |
|
@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.
efa8caa to
1642eed
Compare
|
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. |
|
@llllllllll Which compiler are you using currently (name and version)? |
|
gcc (GCC) 10.1.0 |
|
I added tests to #1053 |
|
This was approved by @jkeiser and we have tests now for C++20, so let us go! |
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
beginandend, except there arealso composable operations like maps, filters, joins, etc.
The iterator objects returned by a range's
beginandendrequire a morestrict set of operations than is needed for a range-for loop.
This PR adds the extra operations needed to support turning
dom::arrayanddom::objectinto 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 isthe 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::arrayanddom::objectsatisfy the range concept. No runtime tests havebeen added yet because these compile time checks should be sufficient.
If desired, the
static_assertcode could be moved out of the actual codeheaders 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::transformand a lambda. Ifkey_value_pairwere 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), thenranges::views::keysandranges::views::valuescould be used directly.