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

Fix using wxLauncher with wxwidgets 3.2 #173

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MageJohn
Copy link
Contributor

@MageJohn MageJohn commented Dec 13, 2022

I was trying to compile wxLauncher with wxwidgets 3.2 (wxgtk3 to be specific). I was hitting this compilation error:

In file included from /usr/include/wx-3.2/wx/wx.h:24:
/usr/include/wx-3.2/wx/event.h: In instantiation of ‘constexpr auto wxPrivate::DoCast(void (C::*)(E&)) [with E = wxHtmlLinkEvent; C = ModInfoDialog]’:
/home/magejohn/Source/wxLauncher/code/controls/ModList.cpp:1400:50:   required from here
/usr/include/wx-3.2/wx/event.h:157:12: error: ‘wxEvtHandler’ is an inaccessible base of ‘ModInfoDialog’
  157 |     return static_cast<void (wxEvtHandler::*)(E&)>(pmf);
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/wx-3.2/wx/event.h:157:12: note:    in pointer to member function conversion

This appears to be because the the ModInfoDialog class inherits from wxDialog, but does so privately. Changing the code to inherit publicly allows wxLauncher to compile with wxwidgets 3.2.

Though this compiles, running the code I encountered assertion errors about argument types not being correct. I tracked them down to some logging calls where the %d format specifier was being used for arguments with the type size_t. size_t is always an unsigned type, but the length varies based on the platform. In theory the fix here is to use a format specifier of %zu, which specifies an unsigned integer with the same length as size_t. However, the z length specifier was only added in wxWidgets 3.1.0, so using it in this case would not maintain compatibility. However, the actual values in these cases will be relatively small, plenty small enough to be held in a regular unsigned integer; therefore I cast them to unsigned and change the specifier to %u.

@MageJohn MageJohn changed the title Fix compiling wxLauncher with wxwidgets 3.2 Fix using wxLauncher with wxwidgets 3.2 Dec 13, 2022
Copy link
Contributor

@Goober5000 Goober5000 left a comment

Choose a reason for hiding this comment

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

FreeSpace Open uses a SIZE_T_ARG macro that is defined on a per-platform basis, like so:

in clang.h, doxygen.h, gcc.h, mingw.h:

#define SIZE_T_ARG    "%zu"

in msvc.h:

#define SIZE_T_ARG    "%Iu"

Is that an option?

@MageJohn
Copy link
Contributor Author

Good question; I think what's happening there is that Visual Studio didn't support the z length modifier before Visual Studio 2015 (reference: 2013 vs 2015 documentation).

In the case of the format strings in this PR though, they're going to logging functions from wxWidgets, and they don't seem to get passed through directly to the platform printf like functions. According to the wxWidgets changelog, support for the z length specifer was added in version 3.1.0; this would be a pretty trivial reason to drop support for older versions of wxWidgets, so I thought the casting method might be the best way.

That said, we could perhaps create a macro that uses z if building with a new enough version of wxWidgets; this might be tricky if we also need to cast the argument when the z isn't available. Alternatively, we could cast the offending size_t arguments to an unsigned long int rather than an unsigned int; that might be more correct than the way I did it, actually.

@MageJohn
Copy link
Contributor Author

Hey @Goober5000, did you have an opinion on what way you wanted me to resolve your feedback?

P.S.

Thanks for your quick response when I initially opened this PR, and my others! I really appreciate that somebody is still helping to maintain this project. Khronoss is cool, but it's still in relatively early stages and wxLauncher is more reliable for now.

@Goober5000
Copy link
Contributor

Sorry, I've been having a pretty busy month! I just took a look at your comment and I'm happy to go with the original version of the PR. The important thing was to make sure we had all available information on which to make a decision.

We don't need to create a macro that uses z; that seems like it would be more work than is warranted. Only one question before I merge this PR: did you want to cast those arguments to unsigned long int?

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.

2 participants