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

Change IO#write, IO#skip, IO.copy to return Int64 #9363

Merged
merged 1 commit into from
May 28, 2020

Conversation

bcardiff
Copy link
Member

This PR

The dilema between using UInt64 or Int64 was due to:

  • OS / libc uses unsigned int under the hood in some places.
    • Odly, I was sure LibC.write returned unsigned, but I was wrong 🤷‍♂️. I know we are not using the turned value directly in IO#write, but they are related.
  • Keeping consistency with existing IO.copy

But since mixing signed and unsigned types will impose some performance issues, we can avoid them if we discourage unsigned int in some cases like this one.

@bcardiff bcardiff added this to the 0.35.0 milestone May 27, 2020
@bcardiff bcardiff merged commit 6380fa9 into crystal-lang:master May 28, 2020
@Sija
Copy link
Contributor

Sija commented May 31, 2020

Heads up: it seems that this change broke sth on nightlies - see https://travis-ci.com/github/Sija/blurhash.cr/jobs/341785188

@bcardiff
Copy link
Member Author

@Sija not by this PR. https://travis-ci.com/github/Sija/blurhash.cr/jobs/340619464 is also broken in nightlies. Some of the dependencies is still not up to date with IO breaking changes.

bcardiff pushed a commit to bcardiff/crystal that referenced this pull request Jun 12, 2020
bcardiff pushed a commit that referenced this pull request Jun 16, 2020
* Revert "Change IO#write, IO#skip, IO.copy to return Int64 (#9363)"

This reverts commit 6380fa9.

* Revert "Make IO#skip IO#write returns the number of bytes it skipped/written (#9233)"

This reverts commit 7f13250.

* Keep IO::Stabled#skip_to_end

* Keep IO#copy returning Int64

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

Successfully merging this pull request may close these issues.

None yet

3 participants