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.

Sign up for GitHub

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

Jump to bottom

Fix fd(::IOStream) returning an integer #55080

Merged
merged 8 commits into from
Jul 25, 2024
Merged

Fix fd(::IOStream) returning an integer #55080

merged 8 commits into from
Jul 25, 2024

Conversation

LilithHafner
Copy link
Member

As suggested by @halleysfifthinc in #54855 (comment)

This PR is to check how breaking it is.

@LilithHafner LilithHafner added the breaking This change will break code label Jul 8, 2024
@LilithHafner
Copy link
Member Author

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@LilithHafner
Copy link
Member Author

PkgEval looks clean...

@LilithHafner LilithHafner added minor change Marginal behavior change acceptable for a minor release and removed breaking This change will break code labels Jul 9, 2024
@oxinabox
Copy link
Contributor

oxinabox commented Jul 9, 2024

RawFD is a 32bit primative.
Int is either 32 bit or 64bit depending on system
But A file descriptor in linux is apprently at most 1023 (https://stackoverflow.com/a/30710778/179081) but i think that's wrong/
It can be changed (https://admhelp.microfocus.com/accurev/en/latest/online/Content/AccuRev-Admin/filehandle_limits.htm) but it is basically capped at max number of open files, so it is never going to bebeyond an Int32 range on a real physical system right now.
So this will not overflow.
So this is fine

And as far as any ccalls we might use, this RawFD looks like an Int.
And as far as any API in julia we have, it would be very odd (and bad) if we had any API that expects and integer/
so this completely fine and safe

Do it

@LilithHafner
Copy link
Member Author

Seeing support and neither breakage nor objections, I'm going to go ahead and merge it. We can revert if it causes unforeseen problems.

fingolfin
fingolfin reviewed Jul 24, 2024
View reviewed changes
base/iostream.jl Outdated Show resolved Hide resolved
fingolfin
fingolfin reviewed Jul 24, 2024
View reviewed changes
base/iostream.jl Outdated Show resolved Hide resolved
This was referenced Jul 24, 2024
Open
Open
LilithHafner and others added 2 commits July 24, 2024 19:22
@LilithHafner @fingolfin
Update base/iostream.jl
c337185 
Co-authored-by: Max Horn <[email protected]>
@fingolfin @LilithHafner
Update base/iostream.jl
fbd2180 
Co-authored-by: Lilith Orion Hafner <[email protected]>
@fingolfin fingolfin added the merge me PR is reviewed. Merge when all tests are passing label Jul 25, 2024
@LilithHafner LilithHafner merged commit bcd9cce into master Jul 25, 2024
6 of 8 checks passed
@LilithHafner LilithHafner deleted the LilithHafner-patch-2 branch July 25, 2024 13:53
@LilithHafner LilithHafner removed the merge me PR is reviewed. Merge when all tests are passing label Jul 25, 2024
@LilithHafner LilithHafner mentioned this pull request Jul 31, 2024
Closed
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Aug 17, 2024
@LilithHafner @fingolfin @lazarusA
Fix fd(::IOStream) returning an integer (JuliaLang#55080)
ac3033c 
As suggested by @halleysfifthinc in
JuliaLang#54855 (comment)

---------

Co-authored-by: Max Horn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin approved these changes

Assignees
No one assigned
Labels
minor change Marginal behavior change acceptable for a minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LilithHafner @nanosoldier @oxinabox @fingolfin