Change return type of wxGetTranslation (PR #26196)
Maarten
Change it from const wxString& to just wxString so there is no need to keep a copy around of untranslated strings.
This allows to remove thread_local UntranslatedStringHolder used by GetUntranslatedString, which causes many problems with MinGW.
Fixes #26195
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26196
Commit Summary
File Changes
(11 files)
- M include/wx/intl.h (12)
- M include/wx/msw/private/uilocale.h (4)
- M include/wx/private/uilocale.h (4)
- M include/wx/translation.h (92)
- M interface/wx/intl.h (10)
- M interface/wx/translation.h (14)
- M src/common/numformatter.cpp (8)
- M src/common/translation.cpp (96)
- M src/msw/datetimectrl.cpp (4)
- M src/msw/utils.cpp (3)
- M src/msw/webview_edge.cpp (2)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26196.patch
- https://github.com/wxWidgets/wxWidgets/pull/26196.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26196)Looks good to me, thanks!
Pinging @vslavik just in case I'm forgetting any reason not to apply this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Václav Slavík
vslavik left a comment (wxWidgets/wxWidgets#26196)Pinging @vslavik just in case I'm forgetting any reason not to apply this.
I don't remember the reasons for this code (maybe git blame can provide hints, but probably not after all that time), but the only ones I can think of are performance (seems unlikely though) and API compatibility back when we unified ANSI and Unicode builds. Plausibly the function returned wxChar* back then and there was a risk of it being stored. If wxGetTranslation returned a copy and its wxChar* conversion was implicitly called, there would be use-after-free.
I don't think this is concern nowadays.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
taler21
taler21 left a comment (wxWidgets/wxWidgets#26196)Such a change needs to be documented as a potential incompatibility in Changes in behaviour not resulting in compilation errors.
For example, in our project there is in some places code like this
const wxString& GetText() const { return wxGetTranslation( ms_texts[m_index] ); }
that needs to be adapted accordingly.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#26196)Good point. I mistakenly thought the changelog was only updated whenever a release is made, but incompatible changes should be added immediately.
Depending on settings, compilers do give a warning or error after this change, like:
error C2220: the following warning is treated as an error
warning C4172: returning address of local variable or temporary
But this is not the default, so putting it in Changes in behaviour not resulting in compilation errors seems correct to me.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26196)I thought compilers would warn about this but if they don't, this change is worse than I thought :-( It's probably still not enough to revert it.
I'll add a note to the changelog.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
taler21
taler21 left a comment (wxWidgets/wxWidgets#26196)@vadz, after this change has been reverted in df0d6be (Revert "Change return type of wxGetTranslation", 2026-05-03), 079b891 (Document change of wxGetTranslation() return type to wxString, 2026-02-18) could/should probably also be reverted.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26196)Thanks for the reminder, will revert too.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.