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

HPyBytes_AsString should return const char* #183

Closed
antocuni opened this issue Mar 10, 2021 · 2 comments · Fixed by #383
Closed

HPyBytes_AsString should return const char* #183

antocuni opened this issue Mar 10, 2021 · 2 comments · Fixed by #383
Assignees
Milestone

Comments

@antocuni
Copy link
Collaborator

antocuni commented Mar 10, 2021

Currently we have:

char* HPyBytes_AsString(HPyContext *ctx, HPy h);
char* HPyBytes_AS_STRING(HPyContext *ctx, HPy h);

these two functions were quickly added by me because I needed them for ujson. They return char * to match CPython's signature, but I think they should return const char* instead.
The reason why CPython's returns char* is this note in the docs:

[...] The data must not be modified in any way, unless the object was just created using PyBytes_FromStringAndSize(NULL, size)

But I don't think we want to support this use case. Instead, we should introduce StringBuilders (see #181 ).

Moreover, this will be consistent with e.g. [H]PyUnicode_AsUTF8: it used to return char*, but since CPython 3.7 it returns const char*

Another question is whether we want to keep the AS_STRING version at all, and all the similar UPPERCASE macros. Ideally not, but on CPython they might be slightly faster, so we need proper benchmarks to make an proper decision.

@hodgestar
Copy link
Contributor

We should also document how long the returned memory is valid (perhaps only as long as the handle?) and add checks to debug mode that it is not written to and that it is not accessed after the handle is closed (or whatever lifetime we pick).

@fangerer
Copy link
Contributor

@hodgestar: Agreed. We already defined that and implemented exactly that check for HPyUnicode_AsUTF8AndSize (see function

const char *debug_ctx_Unicode_AsUTF8AndSize(HPyContext *dctx, DHPy h, HPy_ssize_t *size)
). The infrastructure to do so is already there, so it shouldn't a problem to do the same for HPyBytes_AsString.

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 a pull request may close this issue.

4 participants