Skip to content

Conversation

QuLogic
Copy link
Contributor

@QuLogic QuLogic commented Feb 6, 2022

This appears to have been how it is upstream for about 10 years. Using an interface breaks if a file descriptor number is passed directly to NewFile, e.g., NewFile(3, "fuzz_in") as used in Go 1.18 fuzzing code.

This appears to have been how it is upstream for about 10 years. Using
an interface breaks if a file descriptor number is passed directly to
`NewFile`, e.g., `NewFile(3, "fuzz_in")` as used in Go 1.18 fuzzing
code.
@dkegel-fastly
Copy link
Contributor

Here's when tinygo changed to not use a uintptr directly:
#1012

So the essence of your change is that since file_unix.go and file_other.go use disjoint build constraints, they could go back to sharing uintptr rather than defining separate file handle types...?

@QuLogic QuLogic mentioned this pull request Feb 7, 2022
@dgryski
Copy link
Member

dgryski commented Feb 25, 2022

This seems like a reasonable change, especially if that's what upstream does. Thoughts @aykevl since you were the one who moved it away from a uintptr ?

@QuLogic
Copy link
Contributor Author

QuLogic commented Feb 28, 2022

So the essence of your change is that since file_unix.go and file_other.go use disjoint build constraints, they could go back to sharing uintptr rather than defining separate file handle types...?

Well, I think they do still have to define their own separate file handle types; it's just that the FileHandle type is no longer the public API for it, but uintptr instead.

@aykevl aykevl merged commit a680bfb into tinygo-org:dev Mar 1, 2022
@aykevl
Copy link
Member

aykevl commented Mar 1, 2022

Thank you! Yes, this seems like a reasonable change to improve compatibility.

@aykevl
Copy link
Member

aykevl commented Mar 1, 2022

Somewhat unrelated: I've been thinking we should perhaps remove the whole virtual filesystem thing and instead let application code rely on io/fs (which didn't exist yet when I implemented VFS support). This works for read-only cases but doesn't work if writing to files is also needed.
But this should perhaps be discussed in a different place.

@QuLogic QuLogic deleted the file-uintptr branch March 1, 2022 20:43
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