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

Remove "wprintf" from PAL #77852

Merged

Conversation

AaronRobinsonMSFT
Copy link
Member

The last usage was in FormatMessageW, which
only used wprintf syntax for messages that used
embedded printf format between two "!". We do
not use this feature in any of our messages so
this code path is technically unused.

Remove _snwprintf_s from PAL

Remove _vsnwprintf_s from PAL

_woutput_s was not removed because it is intertwined
with the implementation for printf. The linker will just
prune the implementation anyways.

Remove all wprintf testing from PAL

The last usage was in FormatMessageW, which
only used wprintf syntax for messages that used
embeded printf format between two "!". We do
not use this feature in any of our messages so
this code path is technically unused.
_woutput_s was not removed because it is intertwined
with the implementation for printf. The linker will just
prune the implementation anyways.

Remove all wprintf testing from PAL
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 8.0.0 milestone Nov 3, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title Remove wprint from pal Remove "wprintf" from pal Nov 3, 2022
@AaronRobinsonMSFT
Copy link
Member Author

/cc @am11 @jkotas @davidwrighton

Copy link
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

src/coreclr/pal/tests/palsuite/CMakeLists.txt Show resolved Hide resolved
FormatMessageW errors if embedded formating is used
Update PAL tests for FormatMessageW
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title Remove "wprintf" from pal Remove "wprintf" from PAL Nov 3, 2022
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@AaronRobinsonMSFT
Copy link
Member Author

Issues are known and properly tagged above.

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 8036885 into dotnet:main Nov 4, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the remove_wprint_from_pal branch November 4, 2022 02:25
radical added a commit to radical/runtime that referenced this pull request Nov 4, 2022
This completely broke `dotnet-runtime-perf` pipeline for wasm runs.

This reverts commit 8036885.

Issue: dotnet#77883
@ghost ghost locked as resolved and limited conversation to collaborators Dec 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants