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.

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

Rename file_descriptor to file_description_ref #3808

Closed
tiif opened this issue Aug 16, 2024 · 5 comments · Fixed by #3867
Closed

Rename file_descriptor to file_description_ref #3808

tiif opened this issue Aug 16, 2024 · 5 comments · Fixed by #3867
Labels
A-files Area: related to files, paths, sockets, file descriptors, or handles C-cleanup Category: cleaning up our code E-good-first-issue A good way to start contributing, mentoring is available

Comments

@tiif
Copy link
Contributor

tiif commented Aug 16, 2024

After #3712 renamed previous FileDescriptor to FileDescriptionRef, several variables still use the old name file_descriptor and should be updated to file_description_ref to avoid confusion. This is a simple cleanup task, but I am currently focused on other work, so I am just opening this issue for anyone who might be interested in helping.

@oli-obk oli-obk added the E-good-first-issue A good way to start contributing, mentoring is available label Aug 16, 2024
@RalfJung
Copy link
Member

That's a very long variable name, so we can also consider something like fd. That doesn't disambiguate between "file descriptor" and "file description", but we have types to help make that distinction, so maybe we don't have to repeat this in the variable name as well.

@RalfJung RalfJung added C-cleanup Category: cleaning up our code A-files Area: related to files, paths, sockets, file descriptors, or handles labels Aug 16, 2024
@DeSevilla
Copy link
Contributor

I'm willing to take this on - seems pretty straightforward. From a quick look, there's one variable named exactly file_descriptor which is a FileDescriptionRef (in fs.rs), but several named file_description which are FileDescriptionRef (in fs.rs and fd.rs). Should both cases be renamed, or only file_descriptor?

@tiif
Copy link
Contributor Author

tiif commented Sep 6, 2024

I think there is one in fd.rs

let Some(file_descriptor) = this.machine.fds.get(fd) else {
I personally don't find the naming of file_description confusing, but I am happy to see it being changed if it would be clearer. Maybe fd_ref would work if we were to change it? But I am not really good in naming variables, so cc @RalfJung :)

@DeSevilla
Copy link
Contributor

DeSevilla commented Sep 6, 2024

Sorry, yes, I meant fd.rs for that first one, just mixed up the file names. fd_ref would match up with some other parts of the code, like these two:

let fd_ref = self.new_ref(fd);

let Some(fd_ref) = this.machine.fds.get(fd) else {

We could also just change file_descriptor to file_description if we're keeping that name.

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2024

I did a bunch more renaming in #3867 to hopefully make things more consistent wrt file description vs file descriptor: the latter (of type i32) are now called fd_num. We should only use them in the outer layers of the shims anyway, internally in Miri everything should be file descriptions.

@bors bors closed this as completed in 297e957 Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-files Area: related to files, paths, sockets, file descriptors, or handles C-cleanup Category: cleaning up our code E-good-first-issue A good way to start contributing, mentoring is available
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants