search

emu-russia / pureikyubu

Nintendo GameCube Emulator (WIP)
Creative Commons Zero v1.0 Universal
164 stars 6 forks source link

Some general fixes - Move to a more safe API #126

Closed raphaelthegreat closed 4 years ago

raphaelthegreat commented 4 years ago

Hello, this my first pull request. To get familiar with the project I have tried to do some fixes without touching much of the main components. So this pull request aims to port the code to more modern and standard C++. To achieve this the following things have been done:

  1. Change the compiler from MSVC to clang-cl. clang-cl is based on LLVM and is perfectly compatible with Visual Studio. In order to use it you must install Clang Compiler tools from visual studio installer application. Clang-cl produces way better code than MSVC does, and supports almost all the MSVC features (with warnings though as many of them are not standard C++).

  2. Replace TCHAR/char with std::wstring_view/std::string_view. This is a new C++ 17 feature. std::wstring_view is just a wrapper around wchar_t* with the STL API but without expensive heap allocations that std::wstring might do.

  3. Introduce fmt::format/fmt::sprintf because they have safer templated APIs, fmt is cross platform and they are faster than printf/sprintf.

  4. Replace some of C API with C++ STL like FILE* with std::ifstream / std::vector. std::filesystem is also used as a cross platform way for file handling.

However this pull request should not be merged! The emulator does not work anymore, crashes all the time and text is missing. Any of these changes can be reverted if you dont like them. So I am asking your help in order to fix those issues.

raphaelthegreat commented 4 years ago

Also I forgot to ask if you mind me changing the comment style from // comment to / Comment / I think it looks more pleasing to the eye but if you dont like it I will revert them back to the original

ogamespec commented 4 years ago

Also I forgot to ask if you mind me changing the comment style from // comment to / Comment / I think it looks more pleasing to the eye but if you dont like it I will revert them back to the original

Yes its okay, a matter of taste. Sooner or later, we’ll switch to Doxygen anyway, it doesn’t matter there. Starting a sentence with a small letter in the comments is my old style, now I use normal human sentences everywhere, with a guide to generating readable Doxygen. Also, old comments may be slightly "Runglish" (a mixture of Russian and English), if it gets better in the process into normal English, I also do not mind 😄 Now I’ve adapted to type text in Google Translate, it translates pretty well.

raphaelthegreat commented 4 years ago

I think this is now ready to be merged. The only thing missing is the solution split which for some reason I cannot get it to work. Can you maybe check the code for errors one more time? Here is a summary of the changes I made:

  1. Switch most of TCHAR/char to std::wstring_view/std::string_view.
  2. Replace almost all Win32 file IO code with std::filesystem.
  3. Introduce a new templated string conversion API (String.h)
  4. Replace sprintf with fmt::format/fmt::sprintf for safe string formating.
  5. Use std::stringstream/std::ifstream for file IO for safer file reading.
ogamespec commented 4 years ago

Submodule dolwin added at 4cf70b

What is this thing? :)

ogamespec commented 4 years ago

About the solution split.

Let's run a PR (only the About dialog is left to find out and volatile in Gekko.h TBR), then I will split the solution into different Toolchains.

You can continue to use the solution with Clang, I will continue to use the solution with VSCompiler. Looking from the chat in the discord, you want to go dig into GekkoCore, so the projects will not change globally much. In case of adding new projects / files, I will update all solutions so that they are keep buildable in the master of the main repository.

raphaelthegreat commented 4 years ago

I fixed the issues you mentioned. However I have decided it would be better to switch back to MSVC as the pong mini game has issues with clang-cl. So you dont need to do any solution spit. In addition because I dont plan on being a regular contributor I dont want to enforce such inconvenience. This is your project and I should have respected that you want to use MSVC instead of clang. In regards to Gekko, after looking at it some more, I dont think I have the courage right now to touch the code there as I lack enough experience with JIT compilers and a port like that requires a lot of work and time both of which I cannot dedicate to right now.

ogamespec commented 4 years ago

Thank you for your significant contribution to the cleanliness and portability of the code. 😃