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

Allow Int64 values within IO#read_at #10356

Merged
merged 4 commits into from
Feb 12, 2021

Conversation

Blacksmoke16
Copy link
Member

@Blacksmoke16 Blacksmoke16 commented Feb 3, 2021

Fixes #10032.

The C bindings have this arg typed as SizeT, which from what I know is an unsigned int. Do we need to worry/do anything about the case where an Int64 is passed on a 32bit system where a sufficiently large enough Int64 would overflow a UInt32?

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Feb 3, 2021

  In src/file.cr:822:18
  
   822 | io = PReader.new(self, offset, bytesize)
                      ^--
  Error: no overload matches 'File::PReader.new' with types File, Int32, Int32

Oh, I would have thought they would have been upcasted...

@straight-shoota
Copy link
Member

We'll' need overloads to accept at least Int32 for backwards compatibility. I suppose other types didn't work before.

@asterite
Copy link
Member

asterite commented Feb 3, 2021

Oh, I would have thought they would have been upcasted

Yeah, I would have liked that too...

#9618

Add overload to accept Int32s
@Blacksmoke16
Copy link
Member Author

@straight-shoota Fixed in latest commit.

@asterite Yea that feature would have been perfect here. No reason not to do that that I can think of.

src/file/preader.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota added this to the 1.0.0 milestone Feb 4, 2021
@bcardiff bcardiff merged commit 4204ebf into crystal-lang:master Feb 12, 2021
@Blacksmoke16 Blacksmoke16 deleted the read-at-size branch February 12, 2021 20:09
@bcardiff
Copy link
Member

Checking, why is this PR a breacking-change?

@Sija
Copy link
Contributor

Sija commented Mar 11, 2021

@bcardiff it's no more after 37b486c

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.

File#read_at limited to Int32
5 participants