jrincayc / ucblogo-code

Berkeley Logo interpreter
https://people.eecs.berkeley.edu/~bh/logo.html
GNU General Public License v3.0
182 stars 34 forks source link

Compilation failure in wxMain.cpp (line 225) #160

Closed zigggit closed 8 months ago

zigggit commented 1 year ago

UCBLogo 6.2.3, Linux, wxGTK 3.0.5

I get the following error when trying to compile:

wxMain.cpp: In function ‘void getExecutableDir(char, int)’: wxMain.cpp:225:17: error: cannot convert ‘wxString’ to ‘const char’ 225 strncpy(path, executable_dir, maxlen); ^~~~~~
wxString

I know very little about wxWidgets programming, but after looking at some of the documentation for wxString, I changed line 225 to the following:

strncpy(path, (const char*)executable_dir.mb_str(wxConvUTF8), maxlen);

UCBLogo now compiles and seems to work correctly. I don't want to claim this is necessarily the correct or best way to solve this problem, but clearly some change is needed.

Update: I've also tried compiling against wxGTK 3.2.2.1 and can confirm the same problem happens (though from reading the wxWidgets documentation on wxString, I didn't think this would make a difference).

dmalec commented 1 year ago

What compiler & version are you using? My local Linux setup doesn't exhibit this behavior and it doesn't look like the GitHub worker nodes do either, so I'm wondering if it's something specific about the compiler.

zigggit commented 1 year ago

GCC 11.2.0

The wxWidgets documentation does seem to suggest the wxString type isn't a pointer to the string contents, but rather to a data structure that encapsulates the string and other information about it. Are we sure the unmodified strncpy() in wxMain.cpp actually copies the desired text (when it compiles)? Maybe it works because the wxString data structure has the string contents as its first member (but we probably shouldn't depend on it always doing that) and perhaps some compilers realise that and allow the conversion.

dmalec commented 1 year ago

Just to clarify, I'm not intending to make any statements about whether the code should change or not. I'm just looking to gather enough information so I can replicate what you are currently seeing.

I'll also review the wxWidgets docs to refresh my understanding of wxString.

dmalec commented 1 year ago

Ah, interesting, so I think I'm now looking at the documentation you looked at, was it this section? https://wiki.wxwidgets.org/Converting_everything_to_and_from_wxString#wxString_to_char.2A

zigggit commented 1 year ago

That looks like a page I read a few hours ago.

zigggit commented 1 year ago

I have been experimenting further...

It turns out the wxWidgets libraries I was using were compiled with the option --enable-stl. I don't know whether this is the default for wxWidgets , though due to the issue at hand, I suspect it isn't. If I compile a version of wxWidgets without this option, then UCBLogo compiles and runs without the modification in wxMain.cpp I described earlier (it compiles with the change in place too.)

I hypothesise that the representation of wxString changes depending on whether or not the STL option was used to compile wxWidgets. It would probably still be a good idea to use a proper conversion to make UCBLogo usable in both situations and to protect against any future changes in wxWidgets that alters the representation of wxString.

ryandesign commented 10 months ago

This problem was also reported by a user running macOS Sonoma: https://trac.macports.org/ticket/68673

jrincayc commented 8 months ago

This looks like a reasonable change, so I created a pull request that uses this fix.