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

Externals: Update fmt to 9.1.0 #11123

Merged
merged 4 commits into from
Nov 23, 2022
Merged

Conversation

Pokechu22
Copy link
Contributor

This is mostly a test; I'm not aware of any benefits of updating at this time.

@lioncash
Copy link
Member

lioncash commented Oct 6, 2022

Generally, you'd want to update anyway, since fmt 9.0.0 deprecates implicitly converting unscoped/regular enums into ints to make behavior consistent with scoped enum behavior. So all updating later does is potentially make things break since the deprecated facilities will likely be removed outright on the next major version.

@Pokechu22
Copy link
Contributor Author

fmt 9.0.0 deprecates implicitly converting unscoped/regular enums into ints to make behavior consistent with scoped enum behavior

Indeed, that seems to be the cause of most of the errors. I've worked around that for now (mostly by excessive casts - I can convert to EnumFormatter later).

The other issue is that Qt creates a uint typedef in the global namespace, which ends up being shadowed by this in fmt. I've worked around this by disabling the warning, but I'm not sure of a better solution (I don't think we can move Qt to a separate namespace using QT_NAMESPACE easily since that would break shared libraries.)

@Pokechu22 Pokechu22 force-pushed the fmt-9.1.0 branch 3 times, most recently from 3a4c14d to e7917d5 Compare October 13, 2022 00:36
@@ -3,6 +3,11 @@

#include "DolphinQt/Config/GraphicsModListWidget.h"

#ifdef _MSC_VER
// Disable warning for hiding global declaration (qt uint vs fmt uint)
#pragma warning(disable : 4459)
Copy link
Contributor

Choose a reason for hiding this comment

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

we have a fix for this in fmt, please keep it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't see them because they weren't listed as patch files... I assume 357dc72 and 7b8e6c5?

Copy link
Contributor

Choose a reason for hiding this comment

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

yup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like 357dc72 isn't needed anymore, but I was able to cherry-pick 7b8e6c5 and it seems to work.

@Pokechu22 Pokechu22 marked this pull request as ready for review October 18, 2022 22:34
@lioncash
Copy link
Member

Is this good to be merged?

@Pokechu22
Copy link
Contributor Author

Theoretically there could be some additional enums converted to enum classes, but that can also be done later on (and some of these, such as the IOS error codes, are trickier to change). So this is probably good to go now.

We can't convert fmt to a submodule yet due to the extra patch. However, it looks like an equivalent same patch was adopted as fmtlib/fmt#3137, so we'd be able to use a submodule after that (although maybe fmt.vcxproj would still cause issues?)

@lioncash lioncash merged commit d6437b7 into dolphin-emu:master Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants