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

Added simple polygon validity checks. #32

Merged
merged 5 commits into from Aug 31, 2018
Merged

Added simple polygon validity checks. #32

merged 5 commits into from Aug 31, 2018

Conversation

julienjpk
Copy link
Contributor

This commit fixes #31 and prevents the library from allocating invalid amounts of memory or
dereferencing invalid pointers. Obviously, we'd except users to not pass invalid polygons in the first place, but well... Better safe than sorry.

Please review the changes before merging if you plan to do so: I've made sure the code compiles but haven't tested it. There shouldn't be much room for error though.

This prevents the library from allocating invalid amounts of memory or
dereferencing invalid pointers.
@ivanfratric
Copy link
Owner

Thanks for submitting! A couple of comments:

  • I am really not a fan of C++ exceptions. Instead of throwing an exception, I think the functions should return 0 instead (note that the return value of 0 indicates a failure for all triangulation and partition function).

  • If the only check is the point count, then for performance reasons I think it's better to just do
    if(poly->numpoints < 3) return 0;
    instead a validate() call.

@julienjpk
Copy link
Contributor Author

Done. I've kept a Valid method to avoid code duplication should the library ever include more tests (some maximum number of points, some checks on the coordinates, ...).

@ivanfratric
Copy link
Owner

In that case, what do you think about moving the Valid() method (for now) to polypartition.h and declaring it as inline? No performance loss in that case.

@julienjpk
Copy link
Contributor Author

julienjpk commented Aug 31, 2018

I've just made the change. Little note however: modern compilers have their own heuristics to determine whether or not a function should be inlined and are free to ignore an inline keyword whenever. In this case, it appears g++ does not decide to inline Valid, even at -O3 (which includes -finline-functions). The hint does make it happen though.

(Sorry for the useless commits)

@ivanfratric ivanfratric merged commit 4921a9b into ivanfratric:master Aug 31, 2018
@ivanfratric
Copy link
Owner

Thanks!

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.

Invalid memory access case when invalid polygons are given
2 participants