-
-
Notifications
You must be signed in to change notification settings - Fork 590
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
SCons: Disable C++ exception handling by default #1216
SCons: Disable C++ exception handling by default #1216
Conversation
c32aaca
to
7638b77
Compare
Thanks! Given that this was merged in Godot, it makes sense that we do it here too! The tests pass, which is good. Skimming the Godot changes, the scons changes here look comparable. The one thing I'm worried about is if it's easy enough for a GDExtension that's using godot-cpp's SConstruct file to disable "disable_exceptions", since this isn't really an end-developer setting, but something the extension author want to set. @adamscott has been adding a number of changes along this line, and I'd like to try and test this when I get a chance. Unfortunately, I don't know cmake and can't really review those changes. |
Here, the question is: "Are exceptions supported by gd-extension". If so, we should make these changes optional. If not, let's go. |
Whether or not to use exceptions depends on the specific GDExtension. If the GDExtension is integrating a 3rd party library into Godot that requires using exceptions, then they need to be enabled. Or, if the author of the GDException just likes exceptions and wants to use them in their own code. So, I don't think folks will really use the command line argument: the particular GDExtension either needs exceptions or it doesn't. It'll really be the GDExtension author who is deciding to enable or disable them, and so I just want to make sure it's easy enough to do when the GDExtension's |
I haven't tested it, but in theory you should be able to just do |
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 didn't test the cmake changes, but they look OK. But I did test with scons and it didn't work without making a small change - see the code comments.
I also tried putting this in a GDExtension:
env['disable_exceptions'] = False
The code in godot-cpp still compiled (after making the CXXFLAGS
change) with -fno-exceptions
flag, but the user code in the GDExtension didn't, which is actually probably what we want, so I think this fine in that regard.
Fabio and I did some testing, and we think the However, this will need to be rebased now that this code has moved to @akien-mga Are you still interested in working on this one? Or, should one of us take it over? |
I can update it tomorrow. I'll also have to update upstream Godot accordingly if we change it to |
7638b77
to
37af2bb
Compare
Rebased. Didn't compile to test though. |
Following discussion in godotengine/godot-cpp#1216.
I tested this with scons and it does add However, I wasn't able to find a way to re-enable exceptions in a env = SConscript("godot-cpp/SConstruct")
env['disable_exceptions'] = False ... and: env = Environment()
env['disable_exceptions'] = False
Export('env')
SConscript("godot-cpp/SConstruct") ... and a bunch of variations on that, like: SConscript("godot-cpp/SConstruct", exports=['env']) ... and: SConscript("godot-cpp/SConstruct", exports={'disable_exceptions': False'}) But nothing I tried allowed me to pass that option down into godot-cpp's |
@dsnopek the second option, i.e.: env = Environment()
env['disable_exceptions'] = False
Export('env')
SConscript("godot-cpp/SConstruct") should work after applying my suggestion above |
37af2bb
to
354764b
Compare
354764b
to
bf1c03a
Compare
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.
It's working great in my testing now! Thanks!
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.
LGTM 👍
Thanks! |
Following discussion in godotengine/godot-cpp#1216.
Cherry-picked for 4.1 in PR #1281 |
Following discussion in godotengine/godot-cpp#1216. (cherry picked from commit 3bfcbe7)
Counterpart to godotengine/godot#80612.
Didn't test it, neither SCons nor CMake changes.