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 to fmt 8.1.1 #10367

Merged
merged 13 commits into from Jan 20, 2022
Merged

Update to fmt 8.1.1 #10367

merged 13 commits into from Jan 20, 2022

Conversation

Pokechu22
Copy link
Contributor

@Pokechu22 Pokechu22 commented Jan 13, 2022

Fixes issue 12798. Fmt 8.1.1 uses compile-time format string checking by default if consteval is supported (i.e. C++20 is in use), which is the case for MSVC (since we don't restrict the C++ version on MSVC for some reason, though we do on other compilers). There are a large number of changes needed to fix compile errors for that. Otherwise, the main source of errors is enum classes no longer being implicitly converted to ints; in most cases I've added a cast (but I created an EnumFormatter for WD's command enums because it seemed useful there).

Note that the last time I updated a library (#10188), the windows builds broke due to a bad build cache. I don't think that applies here (as fmt doesn't actually have any cpp files; we built it as a header-only library), but it's something to look out for.

@Pokechu22 Pokechu22 force-pushed the fmt-8.1.1 branch 2 times, most recently from 878b405 to c76b741 Compare January 13, 2022 07:53
@Pokechu22
Copy link
Contributor Author

I'm not sure what's going on with the debian builders failing to find fmt::runtime; maybe they're using an older system fmt instead of the updated local one in Externals?

@Scrumplex
Copy link

maybe they're using an older system fmt instead of the updated local one in Externals?

Yep. If you look at line 40 (when expanding the log for shell_1) you can see this in the cmake output:

Using shared fmt 7.1.3

@BhaaLseN
Copy link
Member

[...] which is the case for MSVC (since we don't restrict the C++ version on MSVC for some reason, though we do on other compilers).

This was a longer discussion in various PRs (with the most documenting one being #8856 I guess) with the conclusion that we want at least one compiler to tell us about potential standard issues in advance; and MSVC just happened to fit the bill (IIRC because the earliest ones where we wanted to use C++14 or C++17 didn't have a formal switch for it, and we just said "use latest" instead)

We will be using fmt::runtime, which was added in fmt 8.0.0.
This format string is by definition dynamic and can't be checked at compile time.  There are other similar strings in the log handler and in asserts, but they use vformat and thus don't need fmt::runtime.  We might be able to do a similar thing where the untranslated string is compile-time checked, but FmtFormatT is used in so few places that I don't want to handle that in this PR.
At least in MSVC (which is not restricted from targetting C++20), these can be resolved to either std::format_to or fmt::format_to (though I'm not sure why the std one is available).  We want the latter.
Using unsigned char* or signed char* results in a deprecation warning, which is treated as an error.  It needs to be casted to regular char* for it to work.
@dolphin-emu-bot
Copy link
Contributor

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

  • burnout2-vehicletextures on mvk-osx-m1: diff
  • ea-pink on mvk-osx-m1: diff
  • inverted-depth-range on mvk-osx-m1: diff
  • nfsu-purplerect on mvk-osx-m1: diff
  • rs2-skybox on mvk-osx-m1: diff
  • rs3-bumpmapping on mvk-osx-m1: diff
  • tla-menu on mvk-osx-m1: diff

automated-fifoci-reporter

@Pokechu22
Copy link
Contributor Author

I've updated the minimum fmt version in CMakeLists.txt to 8.0, which has resulted in the debian builder using the fmt in Externals.

Copy link
Member

@jordan-woyak jordan-woyak left a comment

Choose a reason for hiding this comment

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

Changes look good.

@JosJuice JosJuice merged commit 7b8e846 into dolphin-emu:master Jan 20, 2022
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants