Use 'cpointer' instead of 'cstring' where applicable #1320
Conversation
| data.cpointer(), | ||
| data.space(), | ||
| addressof again | ||
| ) |
There was a problem hiding this comment.
This is probably the style I would use on my personal code, but I think it doesn't match the Pony standard library style of only using line breaks where strictly necessary in argument and parameter lists.
| @@ -101,14 +100,36 @@ actor Main | |||
| _set(0, 0) | |||
| else | |||
| _size = len | |||
| _alloc = alloc.max(_size + 1) | |||
There was a problem hiding this comment.
At first glance I think this does the wrong thing in the overflow case (_size == USize.max_value()).
| only with C-FFI functions that return pony_alloc'd character | ||
| arrays. The pointer is scanned for the first null byte, which will | ||
| be interpreted as the null terminator. If a null pointer is given | ||
| then an empty string is returned. |
There was a problem hiding this comment.
I think the docstring should warn about the dangers of passing in a pointer that has no null byte within its bounds. Specifically, it seems that doing so would violate memory safety. Obviously this can only be done via FFI, but it should still be documented. The previous implementation of from_cstring also gave a warning about memory safety if the len argument was not given, and there was no null byte in the scan.
|
Thank you again for spearheading this RFC - I'm much happier with this solution than with the |
|
The crypto tests are failing because of #1319 (closed now). |
|
@malthe can you rebase your branch off of master so this can be restested with issue-1309 in place? While you are doing that, can you squash the 4 commits to 1? |
|
Looks like rebasing didn't stop the |
|
This also needs a CHANGELOG entry |
|
@malthe 0.5.1 was released last night. The CHANGELOG needs to be updated to reflect that, you currently have your change in the section that is already 0.5.1 on master. You'll need to rebase against it. |
malthe
force-pushed
the
issue-1309
branch
5 times, most recently
from
b2a563b
to
f75f6ad
Compare
SeanTAllen
added
needs discussion during sync
and removed
needs discussion during sync
labels
This closes #1309.