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

update fmt and fix warnings that popped up with vs 17.2 #10652

Merged
merged 6 commits into from May 13, 2022

Conversation

shuffle2
Copy link
Contributor

@shuffle2 shuffle2 commented May 10, 2022

This PR fixes some warnings/errors occurring because of fmt after updating to latest VS.

This is expected to not build on windows buildbot for now (buildbot checks for updates every saturday, and i haven't manually updated it). So, we'll start seeing this build break in a few days (and then this PR will work).

Making the PR now in case there needs to be some discussion about the changes.

ping @lioncash because of heavy c++/fmt stuff

as an aside, can we just use <format> on msvc? As of VS 17.2 it's officially standard-compliant™️ (https://devblogs.microsoft.com/cppblog/msvcs-stl-completes-stdc20/)
It seems like the API (at least the subset we use) should be basically compatible, it's just annoying because it's in a different namespace.

make_args_checked is deprecated
see fmtlib/fmt#2760 and the linked comment
@iwubcode
Copy link
Contributor

as an aside, can we just use on msvc? As of VS 17.2 it's officially standard-compliant

I mean you might be able to, but I generally ask why? In a general sense, unless a project is trying to limit includes or doesn't want the hassle of setting up the dependency, fmt provides additional features. I'd even question if fmt is faster, though I have nothing to backup that claim.

More specific to Dolphin, GCC 12.1 just released and still doesn't support std::format. Is there a reason it'd be worthwhile to add more platform differences?

@shuffle2
Copy link
Contributor Author

shuffle2 commented May 11, 2022

i dont know if you refer to fmt being faster for compile or runtime. I haven't tested either.
The main motivation would just be to move to C++ standardized version.
Dolphin does not use any irreplaceable features of fmt / features that don't have equivalence in std (afaik). The closest I could find is 1 file that uses fmt::sprintf, which it probably shouldn't anyway.

edit: i guess we do use FMT_COMPILE, but that's it..

@iwubcode
Copy link
Contributor

iwubcode commented May 11, 2022

@shuffle2 - we also use memory_buffer

@shuffle2
Copy link
Contributor Author

shuffle2 commented May 11, 2022

memory_buffer doesn't really seem important / lacking an equivalent

I'm also not familiar enough with std impl to know if FMT_COMPILE is really lacking.

@shuffle2
Copy link
Contributor Author

besides <format>, open question is about if fa17153 is the correct way to resolve the deprecation.

@sepalani
Copy link
Contributor

i dont know if you refer to fmt being faster for compile or runtime. I haven't tested either. The main motivation would just be to move to C++ standardized version. Dolphin does not use any irreplaceable features of fmt / features that don't have equivalence in std (afaik). The closest I could find is 1 file that uses fmt::sprintf, which it probably shouldn't anyway.

edit: i guess we do use FMT_COMPILE, but that's it..

I do use named arguments in one of my PRs which doesn't have an equivalent, AFAICT.

besides <format>, open question is about if fa17153 is the correct way to resolve the deprecation.

I'm pretty sure it is since C++20 compile-time strings checks are enforced by default according to the documentation. That's why fmt::runtime needs to be used on runtime strings and fmt::make_args_checked is redundant.

Regardless, the code looks good to me.

Copy link
Member

@lioncash lioncash left a comment

Choose a reason for hiding this comment

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

Code looks good. Personally I'd opt for continuing to use fmt until <format> is supported on all compilers we support and then switch off of it, since there are a few API differences between the two

@lioncash
Copy link
Member

Seems like the Windows builders need an updated VS version

@shuffle2
Copy link
Contributor Author

Seems like the Windows builders need an updated VS version

yup, i was just going to wait for the builder to auto-update this weekend

@Tilka Tilka merged commit fcb3f9e into dolphin-emu:master May 13, 2022
10 checks passed
@shuffle2 shuffle2 deleted the fmt branch May 13, 2022 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants