Skip to content

Return WTF-16 from W-suffixed functions instead of converting to WTF-8 #23657

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mpfaff
Copy link
Contributor

@mpfaff mpfaff commented Apr 26, 2025

This was suggested here and is another step towards fixing #23643.

These changes decrease the stack usage of realpathW by 64 KB.

I took the liberty of additionally documenting that realpathW will never access the pathname after it starts writing to out_buffer and took advantage of that to avoid moving the WTF-16 buffer into many call-sites. If this is undesirable, I can remove those particular changes.


As an aside, I noticed that QueryObjectName silently aligns up its buffer to @alignOf(OBJECT_NAME_INFORMATION) (which has the same alignment as pointers). Should this be documented on the function and in functions that call it so that callers know that using a std.os.windows.PATH_MAX_WIDE length buffer could technically fail with error.NameTooLong?

@mpfaff mpfaff force-pushed the realpathW-no-convert branch from 19aacf3 to 74a493e Compare April 26, 2025 01:56
@squeek502
Copy link
Collaborator

To follow the standard deprecation procedure:

  • realpathW should remain for one release cycle, but with a doc comment noting that it is deprecated
  • Add a realpathW2 function with the new implementation and use that
  • After one release cycle, realpathW2 will be renamed to realpathW

Here's a previous example: #19844

@mpfaff
Copy link
Contributor Author

mpfaff commented Apr 26, 2025

I see that readLinkW has the same behaviour, converting the output to WTF-8. Would you like me to cover that in this PR as well, or leave it for another?

Regardless, here are some preliminary notes about changes to that function:

I think we would want to modify std.os.windows.ntToWin32Namespace by replacing the PathSpace return type with an out_buffer: []u16 parameter and []u16 return type. This way we would be able to modify the functions all the way up the chain to readLinkW to use out_buffer: []u16 buffer without needing that intermediate PathSpace on the stack. If I'm understanding the ntToWin32Namespace function correctly, we could easily return error.NameTooLong if out_buffer is not long enough (path.len - 3 or path.len - 6 depending on branching).

All in all, I believe that would require deprecating and adding replacements for 5 functions:

  • std.os.windows.ntToWin32Namespace
  • std.os.windows.ReadLink
  • std.fs.Dir.readLinkW
  • std.posix.readLinkW
  • std.posix.readlinkatW

@squeek502
Copy link
Collaborator

If the readLink changes are more involved, might be better to do those in a separate PR.

@mpfaff mpfaff force-pushed the realpathW-no-convert branch from 1bc5893 to 5ac4113 Compare April 26, 2025 13:11
Copy link
Collaborator

@squeek502 squeek502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, noticing/utilizing the ability to re-use the buffer for input/output of realpathW2 is a nice touch.

(the CI failure is unrelated to this PR, will re-run it once the CI server is restarted)

@mpfaff mpfaff force-pushed the realpathW-no-convert branch from 5ac4113 to 2569835 Compare April 26, 2025 21:51
@mpfaff mpfaff marked this pull request as ready for review April 26, 2025 21:53
@mpfaff mpfaff force-pushed the realpathW-no-convert branch from 2569835 to fcf8123 Compare April 27, 2025 12:24
@squeek502
Copy link
Collaborator

(no need to continually rebase btw, if the CI is all green then you can just wait for a core Zig team member to review it. If there's an unrelated failure on one or two CI runners, then those can be manually restarted without restarting all the rest of the runners [but all the runners need to have finished before the failing one(s) can be restarted])

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.

2 participants