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

Upgrade fmt to 11.0.2 #16007

Merged
merged 9 commits into from
Aug 8, 2024
Merged

Upgrade fmt to 11.0.2 #16007

merged 9 commits into from
Aug 8, 2024

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Sep 20, 2023

Between fmt 7.1.3 and 11.0.2 a lot has happened. wchar_t support is
now more limited and implicit conversions 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 ✅
  • Settings page opens ✅

@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
Copy link
Member Author

lhecker commented Sep 20, 2023

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

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
@lhecker lhecker force-pushed the dev/lhecker/16000-fmt-update branch from d283b28 to d9a8198 Compare July 11, 2024 23:39
@lhecker lhecker force-pushed the dev/lhecker/16000-fmt-update branch from d9a8198 to f651930 Compare July 12, 2024 01:30
@lhecker lhecker force-pushed the dev/lhecker/16000-fmt-update branch from f651930 to 02a8dbd Compare July 20, 2024 00:59
@lhecker lhecker force-pushed the dev/lhecker/16000-fmt-update branch from 02a8dbd to 5c2645d Compare July 20, 2024 01:00
src/inc/LibraryIncludes.h Fixed Show fixed Hide fixed

This comment has been minimized.

@DHowett
Copy link
Member

DHowett commented Jul 22, 2024

Minor issue in that fmt 11 isn't in the vcpkg ports repo yet. That should be an easy fix. :)

@DHowett
Copy link
Member

DHowett commented Jul 22, 2024

microsoft/vcpkg#39738

@DHowett DHowett changed the title Upgrade fmt to 10.1.1 Upgrade fmt to 11.0.2 Jul 22, 2024
vcpkg.json Outdated Show resolved Hide resolved
src/terminal/adapter/DispatchTypes.hpp Show resolved Hide resolved
src/inc/LibraryIncludes.h Show resolved Hide resolved

This comment has been minimized.

src/cascadia/TerminalSettingsEditor/Utils.cpp Outdated Show resolved Hide resolved
@lhecker lhecker marked this pull request as ready for review August 5, 2024 15:31
@lhecker
Copy link
Member Author

lhecker commented Aug 5, 2024

I reviewed the entire diff again and I believe it's correct.

However, I just noticed that this contains all of my sstream --> fmt and c_str() --> string_view changes as well. Should I split them out? What do you think? 🤔

@DHowett
Copy link
Member

DHowett commented Aug 5, 2024

However, I just noticed that this contains all of my sstream --> fmt and c_str() --> string_view changes as well. Should I split them out? What do you think? 🤔

i'm OK keeping it honestly

@DHowett DHowett requested a review from zadjii-msft August 5, 2024 20:43
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Nice! Thanks!

@DHowett DHowett merged commit 9ab2870 into main Aug 8, 2024
20 checks passed
@DHowett DHowett deleted the dev/lhecker/16000-fmt-update branch August 8, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Latest compilers emit a ton of C4996 warnings as errors
4 participants