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

[C++] Better support optional start/stop in "utf8_slice_codeunits" kernel #34929

Open
jorisvandenbossche opened this issue Apr 6, 2023 · 3 comments

Comments

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Apr 6, 2023

Describe the bug, including details regarding any error messages, version, and platform.

There have been various (slightly different) bugs reported about using "utf8_slice_codeunits" with optional start or stop. The stop argument is already optional and translated into the largest int to indicate to always slice until the end, but that internal "workaround" also produces some bugs in the current implementation due to integer overflows.

Potentially, we could use a different mechanism to signal a default start/stop, such as using std::optional<int64_t> instead of std::numeric_limits<int64_t>::max()

Listing the related issues:

The option class is also used for "binary_slice" kernel.

Component(s)

C++

@pitrou
Copy link
Member

pitrou commented Jul 11, 2023

cc @benibus . This would slightly break the C++ API so we have to make sure this would make things better.

@benibus benibus self-assigned this Jul 11, 2023
@benibus
Copy link
Collaborator

benibus commented Jul 13, 2023

If we were to modify SliceOptions directly then that would also affect the ascii kernel as well. Not opposed to doing that, just wanted to give a heads-up.

In any case, I'll probably try this with a distinct options class first to see how it goes with one of the kernels. Then we can determine if the breaking change is justified.

@pitrou
Copy link
Member

pitrou commented Jul 16, 2023

If we were to modify SliceOptions directly then that would also affect the ascii kernel as well. Not opposed to doing that, just wanted to give a heads-up.

I think it's fine to affect both kernels, and actually it's quite logical as well.

If std::optional had been available to us before, we would probably have used it from the start here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants