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 Clipper with tools=no and patch it to auto-disable exceptions #29003

Merged
merged 1 commit into from May 22, 2019
Merged

Build Clipper with tools=no and patch it to auto-disable exceptions #29003

merged 1 commit into from May 22, 2019

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented May 19, 2019

Prerequisite for #28987.
Reverts #17319.
Helps #25262 (needs testing) seems to be fixed for Clipper but other thirdparty code with similar problem detected (assimp), which also uses exceptions. already fixed by #29032.

@Xrayez Xrayez marked this pull request as ready for review May 19, 2019 13:52
@Xrayez Xrayez requested a review from akien-mga as a code owner May 19, 2019 13:52
@Xrayez
Copy link
Contributor Author

Xrayez commented May 19, 2019

Ok, got this tested and works with sprite editor plugin as before, works alright with #28987 in both release_debug and release targets under Windows. I can squash commits together but as it's touching thirdparty code, waiting for review. "Patch" is generated with git diff against working tree.

@akien-mga
Copy link
Member

I'd prefer not to have to modify upstream code at all, unless we really need to. In this case, this should ideally be implemented upstream in a way that lets us disable exceptions, similar to https://github.com/nlohmann/json/blob/ee4028b8e4cf6a85002a296a461534e2380f0d57/single_include/nlohmann/json.hpp#L518-L530

But as there's little hope of seeing any new Clipper 6.x release, I guess we don't have much choice. Commits should be squashed, either all together or keeping the revert separate; but given that this revert by itself reintroduces a bug, I guess it's better to include it together with the rest of the changes which prevent the bug from happening.

@Xrayez
Copy link
Contributor Author

Xrayez commented May 20, 2019

Angus Johnson is currently working on Clipper 10.0.0. Clipper was written in Delphi originally and translated to C++ pretty much mechanically as the author admits that his C++ knowledge is somewhat limited, so it could take quite some time to propagate changes from upstream for such C++ specific change that he has little knowledge of. I've participated in speeding up Clipper 10.0.0 C++ translation as well so he could delegate this work to me (which would be easier for me just to patch this manually? 😄).

@Xrayez
Copy link
Contributor Author

Xrayez commented May 20, 2019

Not related to 6.4.2 but I've tried to translate these exception fixes ideas already to Angus Johnson here.

@Xrayez Xrayez changed the title Build Clipper 6.4.2 for release and make it use Godot-style assertions Build Clipper with tools=no and patch it to auto-disable exceptions May 21, 2019
@Xrayez
Copy link
Contributor Author

Xrayez commented May 22, 2019

I proposed preliminary changes to upstream here with the patch anyway:
https://sourceforge.net/p/polyclipping/bugs/193/

Reverts "Build polygon clipper only in tools builds" (see #17319)
which allows to build Clipper with tools disabled (release) and because
of that, Clipper has to be patched to optionally disable exceptions in
order to be built on some platforms.

Patched Clipper 6.4.2 to be compiled with exceptions enabled/disabled.
and ensure that Clipper-specific exception macros are defined: don't use
exceptions by default unless exception handling is detected.

Compilation with exceptions will be determined by various
C++ exceptions defines:

* ` __cpp_exceptions` is part of C++ feature testing macros (since C++98);
* `__EXCEPTIONS` is used by some GNU compilers;
* `_CPPUNWIND` is used by MSVC.

The user can override specific exceptions behavior via corresponding
`*_USER` macros (i.e. compiling for embedded systems).
@akien-mga akien-mga merged commit fa5cc1d into godotengine:master May 22, 2019
@akien-mga
Copy link
Member

Thanks!

@Xrayez Xrayez deleted the clipper-6.4.2-exceptions-fix branch May 23, 2019 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants