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

supertux/main: add try-catch for locale initialization. #1601

Merged
merged 2 commits into from Dec 16, 2020
Merged

supertux/main: add try-catch for locale initialization. #1601

merged 2 commits into from Dec 16, 2020

Conversation

ericonr
Copy link
Contributor

@ericonr ericonr commented Dec 9, 2020

Fixes #1564.

@tobbi
Copy link
Member

tobbi commented Dec 14, 2020

Sorry, @ericonr can you please rebase this?

@Semphriss
Copy link
Member

When rebasing, make sure to change boots:: to std:: 🙂

@ericonr
Copy link
Contributor Author

ericonr commented Dec 14, 2020

I'm not sure std::filesystem has the imbue feature? At least I don't see it used here...

@Semphriss
Copy link
Member

Rebasing will tell you what's the exact line to use, normally it should show the correct line alongside your changes.

@ericonr
Copy link
Contributor Author

ericonr commented Dec 15, 2020

@Semphriss the line for boost::filesystem had been completely removed, so I simply left anything similar out of my patch.

@tobbi it's been rebased, but I had to add an additional change, since using std::locale::classic() was wrong. It's all explained in the commit message, I tried to be as thorough as possible.

In order to get it to build here, I also had to fix error_handler.cpp, and I hope the commit and new comment are sufficiently informative.

@ericonr
Copy link
Contributor Author

ericonr commented Dec 15, 2020

It seems the c++17 transition was reverted... What do I do with my PR now? What are the future plans? (for what it's worth, I didn't have any issue linking this version)

@tobbi
Copy link
Member

tobbi commented Dec 15, 2020

I'm sorry about the lack of discussion / coordination here. Please rebase so that it merges cleanly with the current master and then we can merge. Should there be a C++17 support in the future, we'll touch your code again. But currently, the C++17 code fails to build for a lot of the people on the team, thus it getting reverted.

Unfortunately, libstdc++'s locale implementation is platform specific
(unlike libcxx, which has a single universal implementation), and its
"generic" implementation, from 2003, claims to support only the "C"
locale, and raises an exception if one tries to use a locale other than
"C". We catch that exception and allow the application to proceed on
such systems, even though it is very much not ideal.

Also leave note for future code changes.

Fixes #1564.
__GLIBCXX__ indicates that one is using libstdc++, not glibc. The
correct macro for this is __GLIBC__.

Also expand the comment about execinfo inclusion.
@ericonr
Copy link
Contributor Author

ericonr commented Dec 16, 2020

Ok, it's been rebased. I also left a warning about the mistake I had corrected in the previous version.

@tobbi tobbi merged commit 60d9afb into SuperTux:master Dec 16, 2020
@tobbi
Copy link
Member

tobbi commented Dec 16, 2020

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SuperTux crashes due to invalid locale when locale is set to C.UTF-8
3 participants