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

Build issues with recent compilers #18

Open
FtZPetruska opened this issue May 27, 2023 · 5 comments
Open

Build issues with recent compilers #18

FtZPetruska opened this issue May 27, 2023 · 5 comments

Comments

@FtZPetruska
Copy link

With gambatte closing in on 16 years since its first commit, some issues with the older code start to show up on modern compiler (Apple Clang 14, LLVM 16, GCC 13).

Most notably, C++ standard features such as bind1st or mem_fun have been deprecated since C++11, and the mem_fn family of functions have been removed since C++17.

Similar, some of the C code uses K&R style function definition, which is deprecated and is treated as an error in C23.

Lastly, scons is arguably not a very popular build system and the reliance on shell script makes it pretty difficult for Windows users to build it outside of something like Msys2.

Here are a few ideas to address these issues:

  • For C++, switch to using at least the C++11 standard. This revision in particular brings a lot of valuable features notably unique_ptr for automatic memory management, std::chrono for high resolution clocks, lambda expressions to replace the bind1st/bind2nd constructs in std::for_each calls, and nullptr constant. That being said, I think C++17 would be a very good candidate too, mainly for the filesystem API.
  • For C, it seems like the only C code consists of a vendored 3rd party library for zip file manipulation. This could probably be easily rewritten in C++ and would most likely benefit the most from C++17 filesystem API.
  • For the buildsystem, I think CMake would make a great fit for several reasons:
    • It works on all platforms
    • Qt has very good integration for it, it was made the standard for Qt6 with qmake development being halted, and is compatible with Qt5 too
    • It drops the need for shell scripts
    • It becomes trivial to integrate the libgambatte build when building the Qt frontend

Lastly, I would be happy to work on these changes, so let me know what you think!

@CasualPokePlayer
Copy link
Member

CasualPokePlayer commented May 27, 2023

C++11 is used currently. The core was originally written in C++98, and was intended to be extremely portable due to that, although many uses of that are simply not possible and aren't amendable (e.g. long long use, fixed type widths which frankly should be used more for API purposes). The deprecated function use is evidence of this itself, std::function was introduced in C++11, so using bind1st and mem_fun was the C++98 way to do it. C++11 has been around for so long however and brings in so many niceities that I think the entire codebase should switch to using it, and keeps to the want of keeping the core extremely portable (something C++17 would in my opinion fail to do)

CMake would be a very welcome change here, I've grown to like it much more and it's much nicer than scons or hand crafted Makefiles when having multiple targets. It would also allow for easily building with MSVC/clang-cl (although the former I wouldn't care for here, the latter looks very enticing)

That said, I haven't noticed any actual build issues with newer compilers, outside of deprecation warnings due to the above mentioned functions, so I haven't put an priority in revamping everything. Note of course that means ripping out all the build scripts and putting in new ones, which itself is a lot of annoying work.

Also, note that zip support is an optional component, so if it breaks it can be turned off here (and even then it frankly doesn't belong in the core in the first place anyways, that should be a frontend concern). libarchive might be better to use there, anyways

@entrpntr
Copy link

I largely agree with everything you've laid out here. The original gambatte prioritized backwards compatibility with ancient compilers, but there is certainly no compelling need to do so on PSR's end. C++17 seems like a completely reasonable target to me.

I had worked a little on many of these things a few years ago (in addition to using GitHub Actions for testing builds on different OSes and compilers), remnants of which probably only remain here. I had also attempted to convert to CMake at some point; I don't recall how much progress I made, but in any event, taking out scons+qmake would be met with universal support from other devs. (Side note: I believe re-writing qdgbas.py into C/C++ might also remove python as a dev dependency, but that would purely be a luxury at this point.)

Needless to say, contributions like these would be extremely welcome. Our expertise is mostly slanted towards the emulation accuracy side of things, and general C++/compiler/tooling knowledge was lacking. These modernizations would make the codebase a lot easier to work with for other prospective contributors as well.

@entrpntr
Copy link

Only other note: as ridiculous as it sounds, it is hard to overstate how useful support for loading zipped ROMs is in practice. Both anecdotally from other emu devs, and from glancing at the GSR title bars in screenshots/videos shared by runners, breaking zip support would have an impact on a significant number of users.

@CasualPokePlayer
Copy link
Member

CasualPokePlayer commented May 27, 2023

To clarify, I think the core should remain as C++11, while the frontend can be whatever (filesystem stuff should remain frontend side, although even then complex APIs like std::filesystem are really unneeded in this context, all that is done for a file is read all of it and place it in a buffer for internal core functions to handle, nothing more). There isn't anything with C++17 in particularly that strikes me as useful for the core anyways (constexpr maybe?)

ZIP support of course shouldn't be broken (and frankly, it won't be, unless it somehow is impossible to compile the old code which compilers will always be offering a way to compile it anyways), although really should be the responsibility of the frontend rather (gambatte qt). libarchive seems like a good replacement (and as a bonus gives support for more archive types).

@FtZPetruska
Copy link
Author

Thank you everyone for your feedback!

I have started working on the CMake buildsystem, it's coming together pretty nicely. I will open a PR shortly.

That said, I haven't noticed any actual build issues with newer compilers

The actual errors I've encountered were in the Qt frontend, but I figured the core should be updated first.

constexpr maybe?

Constexpr is a C++11 feature too! There has been improvements since, C++14 relaxed restrictions for functions, C++17 allows constexpr if and constexpr lambdas.

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

No branches or pull requests

3 participants