-
Notifications
You must be signed in to change notification settings - Fork 20
Visual Studio compilation fixes #306
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
base: master
Are you sure you want to change the base?
Conversation
|
The fixes now also allow using Xcode |
| -Wno-multichar | ||
| if (MSVC) | ||
| target_compile_options(appleii PUBLIC | ||
| /wd4566 # disable multi-character constant warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally we should fix this code in the upstream repo. can you propose a MR there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me see where it pukes and propose a PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well they use /utf-8 in the upstream repo so here for MSVC I can use:
add_compile_options(/utf-8)
instead of /wd4566, which tracks closer to upstream. But your -Wno-multichar is also /wd4566.
Thoughts?
| #include <string.h> | ||
| #include <stdio.h> | ||
|
|
||
| // For Windows MSVC only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where does this code come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ChatGPT. There are tons of versions of this code online. I would credit someone but the amount of copy-paste in there is quite significant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uhm... originally I was using boost which is well tested and portable. nobody likes it, so now we replace it with some random code which has no guarantee to behave the same as the target getopt. I see it as a regression,not a way forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are committing to a multiplatform codebase. We ahould be using the easiest and most reliable code to do that. Because MSVC doesn't have getopt I hacked together something to make it compile. But if instead we use boost I have no problem, the more reliable and the fewer platform-specific splits the better.
Did people say no to boost? It's big and cumbersome, but multiplatform is big and cumbersome. I really really think we should have ONE multiplatform codebase, with ONE front end that works 100% across all platforms, where any dev on any platform can use the canonical IDE to work on the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try a switch (back?) from argparser.cpp to boost::program_options, removing getopt althogether and it certainly is a better multiplatform solution. Let me know if you want me to include it in the PR while we're at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have moved to getopt recently because the libretro build cannot use boost. let me fix all the rest and then we come back to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick comment: you still use boost::property_tree in common2 so that would also have to go if we're getting off boost 100%
| adjustMouseText(text, length, utf8); | ||
| debuggerTextColored(iColor, utf8); | ||
| std::string utf8(2 * length + 1, '\0'); // worst case is 2-bytes utf8 encoding | ||
| adjustMouseText(text, length, utf8.data()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the issue here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MSVC wants the expression to have a constant value.
So to do dynamic memory allocation I just use an std::string.
#306 Signed-off-by: Andrea Odetti <mariofutire@gmail.com>
#306 Signed-off-by: Andrea Odetti <mariofutire@gmail.com>
#306 Signed-off-by: Andrea Odetti <mariofutire@gmail.com>
|
I've committed all non controversial code changes. |
|
Done |
Here are a number of fixes that will allow
sa2to compile cleanly in Visual Studio.Most of the changes are GCC-specific fixes.
The biggest change is the addition of
getopt.candgetopt.hspecifically for Windows builds.There's also a modification to StdAfx.h which lets CMakeLists disable the speech api when using
sa2 on Windows.