Make create zero out the new buffer #102
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| let buffer = Bigarray.(Array1.create char c_layout len) in | ||
| { buffer ; len ; off = 0 } | ||
|
|
||
| let create len = | ||
| let t = create_unsafe len in | ||
| Bigarray.Array1.fill t.buffer (Char.chr 0); |
There was a problem hiding this comment.
shouldn't we reuse the memset we define ~110 lines below?
There was a problem hiding this comment.
Is there any benefit? I guess memset is only needed if you want to fill a sub-region of an array (fill can only fill the whole thing, but that's what we want here).
There was a problem hiding this comment.
consistency is my argument (I've to keep less symbols in my head when reading the code)
The new `create_unsafe` function can be used if you want to trade safety for speed. Fixes mirage#30
|
OK, it uses |
|
imho good to go (apart from a missing changes entry)! thanks! |
|
I'm yomimono and I approve this PR. |
hannesm
added a commit
to hannesm/ocaml-cstruct
that referenced
this pull request
Jul 7, 2017
there's no need to allocate and fill with zero just to blit afterwards. this is a performance regression which was introduced in mirage#102 (May 2016), when `create` changed its semantics.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The API should be safe by default. The new
create_unsafefunction can be used if you want to trade safety for speed.Fixes #30
See also: mirage/mirage-tcpip#204