Implement RFC 1252 expanding the OpenOptions structure #30872
Conversation
Tracking issue: #30014 This implements the RFC and makes a few other changes. I have added a few extra tests, and made the Windows and Unix code as similar as possible. Part of the RFC mentions the unstable OpenOptionsExt trait on Windows (see #27720). I have added a few extra methods to future-proof it for CreateFile2.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
I feel really uncomfortable about |
| /// | ||
| /// let file = OpenOptions::new().write(true).create_new(true).open("foo.txt"); | ||
| /// ``` | ||
| #[stable(feature = "expand_open_options", since = "1.7.0")] |
There was a problem hiding this comment.
This should probably start out unstable for now.
|
Now that you say that |
|
Do new methods need to be unstable if they have a accepted rfc? |
|
New methods should always start out unstable if possible, even if there is an accepted RFC. |
And mark the new methods as unstable.
| @@ -413,6 +414,9 @@ impl OpenOptions { | |||
| /// This option, when true, will indicate that the file should be | |||
| /// `write`-able if opened. | |||
| /// | |||
| /// If a file already exist, the contents of that file get overwritten, but it is | |||
| /// not truncated. | |||
There was a problem hiding this comment.
Could this clarify that the contents don't get immediately overwritten, only when an actual call to write is made? E.g. emphasize that the cursor is at the beginning so written data will tromp existing data.
Also as a side note could you make sure to format this comment and the ones below to 80 characters in width? (the style of the surrounding module)
|
This looks excellent to me, thanks @pitdicker! |
|
Should I mention it is updated? |
| attributes: c::DWORD, | ||
| share_mode: c::DWORD, | ||
| security_qos_flags: c::DWORD, | ||
| security_attributes: c::LPSECURITY_ATTRIBUTES, |
There was a problem hiding this comment.
I think that this inclusion of a pointer may actually affect whether OpenOptions is Send and Sync, could you add a test to make sure that it's the same after this commit as it is today? (I believe it's both Send and Sync today)
|
Ah yes thanks for the ping (definitely useful as github notifications don't go out for updates to the branch). Just one small nit from me and otherwise I believe this is good to go! |
|
Thanks for the reviews! Ik can do some digging, but am not that good at coding :). I am not sure how to test for |
|
You can probably add something like this to fn _assert_send_sync() {
fn _assert_send_sync<T: Send + Sync>() {}
_assert_send_sync::<OpenOptions>();
} |
Otherwise it is not Send and Sync anymore
|
Good catch, it did lose |
|
Thanks again @pitdicker! Taggings as relnotes as this tweaks the behavior of |
Tracking issue: #30014 This implements the RFC and makes a few other changes. I have added a few extra tests, and made the Windows and Unix code as similar as possible. Part of the RFC mentions the unstable OpenOptionsExt trait on Windows (see #27720). I have added a few extra methods to future-proof it for CreateFile2.
alexcrichton
added
the
relnotes
Tracking issue: #30014
This implements the RFC and makes a few other changes.
I have added a few extra tests, and made the Windows and
Unix code as similar as possible.
Part of the RFC mentions the unstable OpenOptionsExt trait
on Windows (see #27720). I have added a few extra methods
to future-proof it for CreateFile2.