Move FdTable to a common location and split off Unix behavior #4045
Conversation
…ation for Windows FS support
CraftSpider
force-pushed
the
move-fdmap
branch
from
ff8b9b7
to
2a20dba
Compare
|
@sivadeilra - here's one of the PRs related to adding Miri support for Windows FS APIs, RE: #3482 |
There was a problem hiding this comment.
Is anything in this file different from what used to be in fd.rs? If yes, please split this up in multiple commits -- one that just moves code without changing it, and one that changes it. As-is, it is nearly impossible to review this since I'd have to manually compare your new code with the current code.
There was a problem hiding this comment.
I'll double check and split it if there's any difference.
What are you referring to here? |
| } | ||
|
|
||
| /// Represents unix-specific file descriptions. | ||
| pub trait UnixFileDescription: FileDescription { |
There was a problem hiding this comment.
Given that only half the trait moved up to the shared part -- is that worth it? You mentioned elsewhere that read and write are also not ideal for Windows. What kinds of changes did you have in mind to their signature?
I am not convinced yet that having a shared base trait for Unix FDs and Windows handles is a good idea.
There was a problem hiding this comment.
My comment on the other PR noted that I may have been wrong about the signature not being good for Windows. I'm going to try something and confirm.
Separately, your first sentence of this comment is what I was referring to by 'keeping all the functions on the base trait' - I'm not sure if it's worth it to split the trait, or to keep it all as one. I just did it this way because it was relatively simple, but I have no strong opinions.
|
☔ The latest upstream changes (presumably #4016) made this pull request unmergeable. Please resolve the merge conflicts. |
I'm open to instead keeping all the functions on the base trait, but this seemed like an easy way to keep them clearly only intended to be used in the unix shims.
Preparation for Windows FS support, see #4043