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

Make exceptions optional #170

Merged
merged 15 commits into from
Aug 3, 2022
Merged

Make exceptions optional #170

merged 15 commits into from
Aug 3, 2022

Conversation

elalish
Copy link
Owner

@elalish elalish commented Jul 28, 2022

Fixes #141

I've introduced a compiler flag: MANIFOLD_DEBUG that enables exceptions, as well as error and info printing. It is off by default, which means not only are the exceptions compiled out, but it also keeps iostream and such from being included anywhere. This is to help remind us to hide messages behind this flag, as it will fail to compile otherwise.

I also added ManifoldParams(), which allows us to control the verbosity of the Boolean operations at runtime (assuming our debug flag is set). I chose to add a new flag instead of using NDEBUG because that flag does not play well with Thrust and makes things run too slowly.

I've also changed the constructors that take an input Mesh to return an empty Manifold if the input data is bad. I also added Status() to the API to return an enum of the problem with the input Mesh if the result was empty. I also replaced some other small user-input asserts with data sanitation so that the program doesn't have to abort.

I also added a bunch of tests about input validation and fixed a few bugs those revealed.

@elalish elalish self-assigned this Jul 28, 2022
@fire
Copy link
Contributor

fire commented Jul 28, 2022

Amazing!

@elalish
Copy link
Owner Author

elalish commented Jul 28, 2022

@pca006132 I'm still trying to figure out why I'm getting a seg fault in the boolean when I have exceptions turned off. I suspect my macros are misbehaving. I also saw this in Google's C++ style guide. They don't like macros in header files, which is exactly how I've been doing my asserts. I hadn't considered that would escape into our public API. Can you think of a better way?

@elalish
Copy link
Owner Author

elalish commented Jul 28, 2022

Okay, I think I've made sure our ASSERT macro is no longer part of our public header. Still pretty sure I'm not doing this all properly yet...

@pca006132
Copy link
Collaborator

I'm still trying to figure out why I'm getting a seg fault in the boolean when I have exceptions turned off.

Do you mean when the assertion is evaluated to false (which will throw an exception when exception is enabled), the program is aborted? When exception is disabled, throw is not a nop, but calls abort. https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_exceptions.html

Sorry I have been relatively busy recently, I will try to find some time to look into this.

#define ASSERT(condition, EX, msg) \
Assert<EX>(condition, __FILE__, __LINE__, #condition, msg);
#else
#define ASSERT(condition, EX, msg)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pca006132 Replying to you here to give context. When MANIFOLD_DEBUG is not defined, the ASSERT macro (which replaced ALWAYS_ASSERT) should compile out completely so there are no throw statements left. However, it now seg faults, so I think my macro is expanding in a way I don't expect.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I found the problem. I had a side-effect inside one of my ASSERT calls 🙄

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2022

Codecov Report

Merging #170 (f8a4786) into master (a1382f2) will increase coverage by 1.97%.
The diff coverage is 53.33%.

@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
+ Coverage   93.91%   95.88%   +1.97%     
==========================================
  Files          30       30              
  Lines        2793     2626     -167     
==========================================
- Hits         2623     2518     -105     
+ Misses        170      108      -62     
Impacted Files Coverage Δ
samples/src/knot.cpp 100.00% <ø> (ø)
src/collider/src/collider.cpp 100.00% <ø> (ø)
src/manifold/include/manifold.h 100.00% <ø> (ø)
src/manifold/src/boolean3.cpp 100.00% <ø> (ø)
src/manifold/src/constructors.cpp 99.36% <ø> (-0.01%) ⬇️
src/manifold/src/csg_tree.cpp 74.73% <ø> (ø)
src/manifold/src/impl.h 100.00% <ø> (ø)
src/manifold/src/manifold.cpp 87.58% <0.00%> (ø)
src/manifold/src/shared.h 98.07% <ø> (-0.04%) ⬇️
src/polygon/src/polygon.cpp 98.50% <0.00%> (+11.32%) ⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2033bbf...f8a4786. Read the comment docs.

@elalish elalish merged commit c0c0034 into master Aug 3, 2022
@elalish elalish deleted the noExceptions branch August 3, 2022 05:13
cartesian-theatrics pushed a commit to SovereignShop/manifold that referenced this pull request Mar 11, 2024
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

Successfully merging this pull request may close these issues.

Removing exceptions
4 participants