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

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

Closed
zigggit opened this issue Mar 5, 2023 · 8 comments · Fixed by #174
Closed

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

zigggit opened this issue Mar 5, 2023 · 8 comments · Fixed by #174

Comments

@zigggit
Copy link

zigggit commented Mar 5, 2023

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
Copy link
Collaborator

dmalec commented Mar 5, 2023

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
Copy link
Author

zigggit commented Mar 5, 2023

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
Copy link
Collaborator

dmalec commented Mar 5, 2023

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
Copy link
Collaborator

dmalec commented Mar 5, 2023

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
Copy link
Author

zigggit commented Mar 5, 2023

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

@zigggit
Copy link
Author

zigggit commented Mar 6, 2023

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
Copy link

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

@jrincayc
Copy link
Owner

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

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 a pull request may close this issue.

4 participants