Insert FileDescription instead of FileDescriptor in insert_fd
#3763
Conversation
|
@rustbot ready |
tiif
changed the title
insert_fd
|
@bors r+ |
|
☀️ Test successful - checks-actions |
| } | ||
|
|
||
| pub fn insert_fd(&mut self, file_handle: FileDescriptor) -> i32 { | ||
| /// Insert a file descriptor to the FdTable. |
There was a problem hiding this comment.
The comment seems to contradict the signature? This is a file description, isn't it?
| } | ||
|
|
||
| pub fn insert_fd(&mut self, file_handle: FileDescriptor) -> i32 { | ||
| /// Insert a file descriptor to the FdTable. | ||
| pub fn insert_fd<T: FileDescription>(&mut self, fd: T) -> i32 { |
There was a problem hiding this comment.
I'm not sure what the motivation is for this API change, but it seems odd to me. Note that insert_fd_with_min_fd wasn't changed, so now we have two equally-named APIs doing very different things. And insert_fd_with_min_fd can't be changed since indeed it is used to insert duplicates of existing file descriptions that shouldn't be wrapped in a new Rc.
This change is needed by #3712.
I don't understand how it is possible for this change to be needed. This change means that the API is now strictly less powerful than it was before. The only argument I can currently imagine for this change is ergonomics, and I don't agree that's worth it given the API inconsistencies this introduces.
IMO this should be reverted, both insert_fd and insert_fd_with_min_fd should take a FileDescriptionRef.
There was a problem hiding this comment.
The API didn't need to be as powerful as it was, so this seems good (restricting to the use case we need). It's also the only sane design I see for having each FileDescriptionrRef carry a unique id that we can use instead of its address.
There was a problem hiding this comment.
We considered do it in FileDescriptionRef::new, but then that needs access to the InterpCx or at least the FdTable.
There was a problem hiding this comment.
It is pure coincidence that we have no operation which creates a 2nd file descriptor for an existing file description, and calls this function. In fact arguably this here should just call insert_fd.
And having two insert_fd functions that don't even take the same kind of argument seems like an obviously bad API to me, way worse than an API that is slightly more general than what we happen to need right now.
Creating a FileDescriptionRef from a FileDescription should be a completely independent operation from inserting a FileDescriptionRef into the FD table.
There was a problem hiding this comment.
It's also the only sane design I see for having each FileDescriptionrRef carry a unique id that we can use instead of its address.
Just have FileDescriptionRef::new take a reference like ecx: &mut MiriInterpCx<'tcx> and then it can do the ID assignment.
Then we can maybe have fd.insert_new_fd for the common pattern of "create new ref and insert it".
There was a problem hiding this comment.
I guess we could call the functions insert_new_fd and insert_existing_fd where only the latter supports a "minimum file description" and then we don't even have to change their implementation. (Though i think factoring FileDescriptionRef::new into a separate operation, even if it remains private, would still help to make the conceptual structure more clear.)
The PR description should have mentioned the point about FileDescriptionRef::new needing access to global state in #3712, that would have avoided some confusion. :)
Some FileDescriptor/FileDescription refactorings follow-up to #3763 (comment) I opted not to change the method names, as I think they are already pretty good (and the common one is the short name), and the docs should explain what they actually do, but if you feel like the names you proposed would be better, I'll just do that.
Some FileDescriptor/FileDescription refactorings follow-up to rust-lang/miri#3763 (comment) I opted not to change the method names, as I think they are already pretty good (and the common one is the short name), and the docs should explain what they actually do, but if you feel like the names you proposed would be better, I'll just do that.
This PR moves the creation of
FileDescriptorinsideinsert_fd, andinsert_fdnow takes inFileDescriptioninstead ofFileDescriptor. This change is needed by #3712.