-
Notifications
You must be signed in to change notification settings - Fork 356
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
windows: basic support for GetUserProfileDirectoryW #3502
Conversation
)?; | ||
// The Windows docs just say that this is written on failure. But std | ||
// seems to rely on it always being written. | ||
this.write_scalar(Scalar::from_u32(len.try_into().unwrap()), &size)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisDenton something seems to be odd here, or I am misunderstanding something. The docs say about the size pointer
If the buffer specified by lpProfileDir is not large enough or lpProfileDir is NULL, the function fails and this parameter receives the necessary buffer size, including the terminating null character.
So in particular, if the buffer is large enough, it doesn't say that the pointer is changed at all.
But the way std calls this function, it assumes that on success, sz
is updated to the actual size (including the null terminator).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. We shouldn't be doing that if the behaviour isn't documented.
Given that's how it's worked in practice and Rust has apparently been relying on this for a long time, I do think updating the API docs is warranted. However, unless or until that actually happens we should act as if such a documentation change would be rejected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming! I will then land this PR (if I stopped updating the size the tests would fail). I opened an issue to track this: rust-lang/rust#124325.
5cd7832
to
817ee4b
Compare
☔ The latest upstream changes (presumably #3504) made this pull request unmergeable. Please resolve the merge conflicts. |
…et's require that in general
…a non-default function
817ee4b
to
10e8bb8
Compare
@bors r+ |
☀️ Test successful - checks-actions |
Fixes #3499