scp-fs2open / wxLauncher

wxLauncher
Other
26 stars 12 forks source link

Fix using wxLauncher with wxwidgets 3.2 #173

Open MageJohn opened 1 year ago

MageJohn commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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?