Enable file writing#973
Conversation
pvdrz
force-pushed
the
write-shim
branch
2 times, most recently
from
7bc36d6 to
5a05c04
Compare
pvdrz
force-pushed
the
write-shim
branch
2 times, most recently
from
bc4fc4e to
6c36a8c
Compare
|
@bors r+ |
|
📌 Commit 6c36a8c has been approved by |
|
☀️ Test successful - checks-travis, status-appveyor |
| }, | ||
| ) | ||
| this.remove_handle_and(fd, |mut handle, this| { | ||
| // Don't use `?` to avoid returning before reinserting the handle |
There was a problem hiding this comment.
Why do we even remove and re-insert, instead of working with a borrowed handle?
There was a problem hiding this comment.
because of borrowing issues, we'd have to mutably borrow the handle, and then mutably borrow the bytes inside memory.
There was a problem hiding this comment.
Could this be fixed if we made the memory field public, instead of exposing it just via getters?
There was a problem hiding this comment.
My naive opinion is that it would work because memory and machine are different fields of ecx. So you can get a mutable reference to file handle from machine and the mutable reference to the bytes from memory.
| this.memory_mut() | ||
| .get_mut(buf.alloc_id)? | ||
| .get_bytes_mut(tcx, buf, Size::from_bytes(count)) | ||
| .map(|buffer| handle.file.read(buffer)) |
There was a problem hiding this comment.
This line is the actual core of the function, doing the read, right? Please highlight that with a comment. It is kind of burried in bookkeeping.
| let bytes = this.memory().get(buf.alloc_id).and_then(|alloc| { | ||
| alloc | ||
| .get_bytes(tcx, buf, Size::from_bytes(count)) | ||
| .map(|bytes| handle.file.write(bytes).map(|bytes| bytes as i64)) |
There was a problem hiding this comment.
This line is the actual core of the function, doing the write, right? Please highlight that with a comment. It is kind of burried in bookkeeping.
| let bytes = b"Hello, World!\n"; | ||
| // Test creating, writing and closing a file (closing is tested when `file` is dropped). | ||
| let mut file = File::create(path).unwrap(); | ||
| // Writing 0 bytes should not change the file contents. |
There was a problem hiding this comment.
Isn't the main issue here that writing 0 bytes should not require the pointer to be valid?
5 participants
r? @oli-obk