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

Deprecate FutexOperation instead of futex #1118

Merged
merged 7 commits into from
Aug 27, 2024
Merged

Conversation

sunfishcode
Copy link
Member

With rustix::thread::futex being both a deprecated function and a public module, I'm seeing warnings like this when I use it in some simple testcases:

warning: use of deprecated function `rustix::thread::futex`: There are now individual functions available to perform futex operations with improved type safety. See the futex module.
 --> test.rs:2:21
  |
2 | use rustix::thread::futex;
  |                     ^^^^^
  |
  = note: `#[warn(deprecated)]` on by default

I think we can fix this by moving futex to be a top-level module, at rustix::futex. And this also reflects that, strictly speaking, futexes aren't specific to threads and can be used between processes too.

Of course, this leaves the deprecated rustix::thread::futex and the associated types and constants in place.

Also while trying out the API, I found that now that we have a public futex module, using it as futex::wait etc. makes me want to write things like futex::FutexFlags, which feels redundant. I propose we rename it to futex::Flags, and similarly rename futex::FUTEX_WAITERS to futex::WAITERS and so on.

@danielschemmel How does this look to you?

@SUPERCILEX
Copy link
Contributor

Regarding the naming conventions, see #753 (comment). I don't have a strong preference, but I do think we should be consistent. Are we going the module + name route? If so, the 1.0 list should rename stuff like inotify.

@SUPERCILEX
Copy link
Contributor

The modules where we'd probably want to rename stuff are: procfs, mount, inotify, pipe, shm.

@sunfishcode
Copy link
Member Author

fs::inotify and shm do look like they'd be nice to do. I've now added those to #753.

Most functions in pipe don't start with pipe_ so I imagine we want to leave that one as-is. mount has several mount_* things but also a lot of non-mount_* things, so imagine we want to leave that as-is too, unless we should talk about a bigger reorg.

I could go either way on the proc_ prefixes in procfs.

@SUPERCILEX
Copy link
Contributor

My only thought for pipe was PipeFlags. Sounds reasonable for everything else!

@sunfishcode sunfishcode force-pushed the sunfishcode/futex-toplevel branch 2 times, most recently from 16529b2 to f5023bf Compare August 24, 2024 18:30
@sunfishcode sunfishcode force-pushed the sunfishcode/futex-toplevel branch 2 times, most recently from 4f66307 to c0cbf53 Compare August 26, 2024 15:15
With `rustix::thread::futex` being both a deprecated function and
a public module, I'm seeing warnings like this when I use it in
some simple testcases:

```console
warning: use of deprecated function `rustix::thread::futex`: There are now individual functions available to perform futex operations with improved type safety. See the futex module.
 --> test.rs:2:21
  |
2 | use rustix::thread::futex;
  |                     ^^^^^
  |
  = note: `#[warn(deprecated)]` on by default
```

I think we can fix this by moving `futex` to be a top-level module,
at `rustix::futex`. And this also reflects that, strictly speaking,
futexes aren't specific to threads and can be used between processes
too.

Of course, this leaves the deprecated `rustix::thread::futex` and
the associated types and constants in place.

Also while using the API, I found `futex::FutexFlags` to be a little
redundant, so I think it makes sense to rename it to `futex::Flags`,
and similarly rename `futex::FUTEX_WAITERS` to `futex::WAITERS` and
so on.
@sunfishcode sunfishcode force-pushed the sunfishcode/futex-toplevel branch from c0cbf53 to 5177d92 Compare August 26, 2024 15:19
@sunfishcode
Copy link
Member Author

Additional datapoint: the epoll API already uses this style, with epoll::create instead of epoll_create.

@danielschemmel
Copy link
Contributor

I have to say I did not expect the use to already cause the deprecation warning. I thought about the issue for a bit and could not come up with more ideas than to either not mark the futex function as deprecated, or rename the futex module to prevent the name clash.

If you are happy with simply making it a top-level module (and thus add a feature for it), that would be a nice solution from the Futex perspective - however, I could also imagine that you would like to prevent the proliferation of top-level features, in which case a more local renaming might be more appropriate.

This helps avoid adding more top-level module clutter, and futexes
are related to threads; they even have builtin knowledge of TIDs.
This also fixes the semver incompatibility as the original `FutexOperation`
is not `#[non_exhautive]`.
@sunfishcode
Copy link
Member Author

Thinking about this more, I am having more reservations about adding a whole new top-level module for this. And, I figured out that we can just deprecate FutexOperation, instead of the futex function, and it does what we need without causing a deprecation warning on the futex module. I've now implemented that, and am now leaning in that direction.

@sunfishcode
Copy link
Member Author

As an aside, even though I observed above that futexes aren't specific to threads, thinking about it more, they kind of are, just not necessarily threads in the same process. They even have knowledge of TIDs built in. So they are pretty thread-oriented.

@sunfishcode sunfishcode changed the title Move futex to be a top-level module. Deprecate FutexOperation instead of futex Aug 27, 2024
@sunfishcode sunfishcode merged commit 5ab7762 into main Aug 27, 2024
43 checks passed
@sunfishcode sunfishcode deleted the sunfishcode/futex-toplevel branch August 27, 2024 06:25
@sunfishcode
Copy link
Member Author

This is released in rustix 0.38.35.

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.

3 participants