Skip to content

Conversation

rbehrends
Copy link
Contributor

The function readAllBuffer() always returned a string that was a
multiple of the BufSize in length, regardless of how many bytes were
actually read, padding the result with garbage bytes on the last chunk.
This fix properly trims the last chunk to its actual size. It also sets
BufSize to a value less likely to cause unnecessary overhead with C and
OS file I/O.

@dom96
Copy link
Contributor

dom96 commented May 25, 2014

IIRC 4000 is a more optimal buffer size. @Araq told me this at some point.

@rbehrends
Copy link
Contributor Author

I'm mostly interested in getting the bug fixed (so that, e.g., stdin.readAll actually works), so I'm not particularly wedded to the buffer size. That said, it'd surprise me to learn that it's more efficient with a buffer size of 4000 (other than as an artifact of one specific fread() implementation).

The function readAllBuffer() always returned a string that was a
multiple of the BufSize in length, regardless of how many bytes were
actually read, padding the result with garbage bytes on the last chunk.
This fix properly trims the last chunk to its actual size.
@rbehrends
Copy link
Contributor Author

Updated the pull request to remove the change to the buffer size. Benchmarking seems to indicate that speed gains, if any, are negligible.

@Varriount
Copy link
Contributor

Looks ok to me.

Araq added a commit that referenced this pull request May 31, 2014
Fixed readAllBuffer() to avoid adding garbage bytes at end.
@Araq Araq merged commit 6ae4626 into nim-lang:devel May 31, 2014
Clyybber added a commit to Clyybber/Nim that referenced this pull request Mar 7, 2024
## Summary
* Remove  `nkStmtListType`  and  `nkBlockType`  node kinds and replace
the remaining usages of  `nkStmtListType`  with  `nkStmtListExpr` 

## Details
*  `not nil`  is implemented as a  `TypeTrait`  magic and no longer
reaches mirgen
* A workaround for untyped AST in sempass2 has been added to the check
introduced in cef64c4 and accordingly a
`knownIssue`  testcase for the underlying issue

---------

Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
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.

4 participants