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

Tweak rust::Slice to become a C++20 contiguous_range. #1432

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anforowicz
Copy link
Contributor

The main missing piece was that
https://en.cppreference.com/w/cpp/iterator/random_access_iterator requires that "(a + n) is equal to (n + a)", but Slice::iterator only supported a + n and didn't support n + a.

The main missing piece was that
https://en.cppreference.com/w/cpp/iterator/random_access_iterator
requires that "`(a + n)` is equal to `(n + a)`", but `Slice::iterator`
only supported `a + n` and didn't support `n + a`.
@anforowicz
Copy link
Contributor Author

PTAL?

Notes:

  • Motivation: I learned recently that before this commit base::span<T> couldn’t be easily constructed out of rust::Slice<T>, because base::span’s constructor in Chromium requires the input to be a contiguous_range (see here and here).
  • I wasn’t sure where to add static_asserts (in cxx.h vs in tests/ffi/[tests.cc](http://tests.cc/) vs somewhere else). I think having them in cxx.h is useful because it also serves as documentation. (Saying that Slice is a contiguous_range felt like a relatively low-level detail so I didn’t add that to book/src/binding/slice.md.)
  • IIUC cargo test builds things in C++11 mode (?), so as-is there are no regression tests within cxx… :-/. But hopefully the new static_asserts will make it easy to detect and diagnose regressions if they get imported into C++20-based clients (like Chromium). So maybe this is good enough…
    • Ideally CI would test things with C++11, C++17, C++20, etc. I may try to do this as a follow-up, but I feel that I don’t know what I am doing, so let me try that in a separate PR. I tried making some progress here: https://github.com/anforowicz/cxx/tree/cxx20-coverage

@anforowicz
Copy link
Contributor Author

/cc @zetafunction (many thanks for your help with the C++ templates :-) )

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.

1 participant