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.

Sign up for GitHub

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

Jump to bottom

Upgrade fmt to 10.1.1 #16007

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Upgrade fmt to 10.1.1 #16007

wants to merge 2 commits into from

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Sep 20, 2023 •
edited

Between fmt 7.1.3 and 10.1.1 a lot has happened apart from improvements.
wchar_t support is now more limited and implicit conversions from
char* to wchar_t* don't work anymore.

Furthermore, even the non-FMT_COMPILE API is now compile-time checked
and so it fails to work in our UI code which passes hstring format
strings which aren't implicitly convertible to the expected type.
fmt::runtime was introduced for this but it also fails to work for
hstring parameters. To solve this, a new RS_fmt macro was added
to abstract the added std::wstring_view casting away.

Finally, some additional changes to reduce stringstream usage have
been made, whenever format_to, etc., is available.
This mostly affects ActionArgs.cpp.

Closes #16000

Validation Steps Performed

  • Compiles ✅

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Product-Meta The product is the management of the products. labels Sep 20, 2023
lhecker
lhecker commented Sep 20, 2023
View reviewed changes
src/cascadia/TerminalApp/AppActionHandlers.cpp
L"")
};
const auto newTabCommandline = newTerminalArgs ? newTerminalArgs.ToCommandline() : winrt::hstring{};
const auto cmdline = fmt::format(FMT_COMPILE(L"-w -1 new-tab {}"), newTabCommandline);
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed all fmt::format calls to use FMT_COMPILE.
I'm not entirely sure why, but by using FMT_COMPILE a bunch of compile errors went away.

src/cascadia/TerminalApp/AppActionHandlers.cpp
@@ -1078,14 +1075,14 @@ namespace winrt::TerminalApp::implementation
{
if (const auto& realArgs = args.ActionArgs().try_as<SearchForTextArgs>())
{
queryUrl = realArgs.QueryUrl().c_str();
queryUrl = std::wstring_view{ realArgs.QueryUrl() };
Copy link
Member Author

Choose a reason for hiding this comment

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

This PR only includes a few (~10?) of these changes. I did them as a drive-by fix because this PR already addresses formatting issues anyways.

lhecker
lhecker commented Sep 20, 2023
View reviewed changes
src/cascadia/TerminalSettingsEditor/Utils.cpp
@@ -28,7 +28,7 @@ namespace winrt::Microsoft::Terminal::Settings
hstring LocalizedNameForEnumName(const std::wstring_view sectionAndEnumType, const std::wstring_view enumValue, const std::wstring_view propertyType)
{
// Uppercase the first letter to conform to our current Resource keys
auto fmtKey = fmt::format(L"{}{}{}/{}", sectionAndEnumType, char(std::towupper(enumValue[0])), enumValue.substr(1), propertyType);
auto fmtKey = fmt::format(FMT_COMPILE(L"{}{}/{}"), sectionAndEnumType, enumValue, propertyType);
Copy link
Member Author

@lhecker lhecker Sep 20, 2023 •
edited

Choose a reason for hiding this comment

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

If you got here, you're 3/4ths done with everything but ActionArgs.cpp. I'd suggest skipping ActionArgs.cpp first, because it's a bit different from the others.

Copy link
Member

Choose a reason for hiding this comment

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

This change seems dangerous; how did you settle on making it?

"Chesterton's fence"

Copy link
Member

Choose a reason for hiding this comment

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

FWIW: resource keys seem to be case sensitive. That's why we put the enum in title case with the weird "toupper" thing.

I don't know why somebody used the dispreferred form of casting, char(x)

Copy link
Member Author

@lhecker lhecker Sep 20, 2023 •
edited

Choose a reason for hiding this comment

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

char(std::towupper(enumValue[0])) doesn't work anymore, because it now prints an integer (presumably because the builtin char -> wchar support was removed).

I've considered just removing the char cast, but I was curious whether it would work without this cast. I went through every view in the settings UI and none of them threw an exception.

Copy link
Member

Choose a reason for hiding this comment

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

Worth trying on Windows 10 as well 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

But the enum names come from our IDL (through jsoncpp), so why would anything change?

Copy link
Member

Choose a reason for hiding this comment

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

Wut

This relies on specific behavior in the Windows resource manager / MRT core. That has presumably changed a lot between Windows builds 19041 and 22621. 😄

@lhecker
Copy link
Member Author

lhecker commented Sep 20, 2023

Should we hold off on this PR until after the 1.19 release?

DHowett
DHowett requested changes Sep 20, 2023
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Should we hold off on this PR until after the 1.19 release?

Yup! I'll block it.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 20, 2023
@DHowett
Copy link
Member

DHowett commented Sep 20, 2023

I would also prefer to (as mentioned before) switch to vcpkg before introducing another copy/paste of some OSS code 😄

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 20, 2023
@zadjii-msft
Copy link
Member

I'm moving this to draft for now to get it out of the queue.

I would also prefer to (as mentioned before) switch to vcpkg before introducing another copy/paste of some OSS code

We'd all love for vcpkg to like, be a good package manager, but that's probably not a viable reason to block this for now.

@zadjii-msft zadjii-msft marked this pull request as draft December 5, 2023 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett DHowett DHowett requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Meta The product is the management of the products.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Latest compilers emit a ton of C4996 warnings as errors
3 participants
@lhecker @DHowett @zadjii-msft