-
Notifications
You must be signed in to change notification settings - Fork 119
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
fix a bunch of warnings #1194
fix a bunch of warnings #1194
Conversation
will fix the build failures tmr |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1194 +/- ##
==========================================
+ Coverage 92.09% 92.29% +0.20%
==========================================
Files 31 31
Lines 5982 5972 -10
==========================================
+ Hits 5509 5512 +3
+ Misses 473 460 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Thanks!
src/impl.cpp
Outdated
@@ -278,6 +275,8 @@ void Manifold::Impl::DedupePropVerts() { | |||
for (int i : {0, 1, 2}) prop[i] = label2vert[vertLabels[prop[i]]]; | |||
} | |||
|
|||
constexpr int removedHalfedge = -2; |
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.
kRemoveHalfedge
?
@@ -376,6 +376,7 @@ void for_each(ExecutionPolicy policy, Iter first, Iter last, F f) { | |||
typename std::iterator_traits<Iter>::iterator_category, | |||
std::random_access_iterator_tag>, | |||
"You can only parallelize RandomAccessIterator."); | |||
(void)policy; |
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 does this 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.
just to silent some warnings about unused parameters when parallelization is disabled. Not really needed though, I can remove them.
@@ -24,6 +24,10 @@ | |||
#include "manifold/manifold.h" | |||
#include "manifold/optional_assert.h" | |||
|
|||
#ifdef MANIFOLD_DEBUG | |||
#include <iomanip> | |||
#endif |
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 was hoping we could have a single, central place for includes guarded by MANIFOLD_DEBUG. What do you think?
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.
Sounds good, I will see where can we do 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.
I will leave this in another PR later. We may want to reorganize the files a bit, I found that the file names often don't correspond to the functions inside it.
// Absolute error <= 6.7e-5 | ||
float acos(float x) { | ||
float fastacos(float x) { |
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.
This function is renamed because c++ is resolving to the C stdlib acos...
All of them are harmless, just some unused code. And because this doesn't involve the ugly int size_t casts, this is quite clean.