refactor(1089): update index types from int to size_t #1090
Conversation
NickUfer
force-pushed
the
fix-size_t-int-mismatches
branch
from
d7241d6 to
6a8db7b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1090 +/- ##
=======================================
Coverage 91.87% 91.87%
=======================================
Files 30 30
Lines 5882 5883 +1
=======================================
+ Hits 5404 5405 +1
Misses 478 478 ☔ View full report in Codecov by Sentry. |
NickUfer
force-pushed
the
fix-size_t-int-mismatches
branch
2 times, most recently
from
af92593 to
af68c57
Compare
include/manifold/manifold.h
Outdated
| @@ -270,7 +270,7 @@ class Manifold { | |||
| size_t NumVert() const; | |||
| size_t NumEdge() const; | |||
| size_t NumTri() const; | |||
| size_t NumProp() const; | |||
| int NumProp() const; | |||
There was a problem hiding this comment.
Why do you want to turn this into int?
There was a problem hiding this comment.
Because NumProp is of type int: https://github.com/elalish/manifold/blob/f1a86930164b360765c5e04d633aae4d617acdd1/src/impl.h#L37C5-L37C21
There was a problem hiding this comment.
I think the right thing is to update that to size_t rather than vice-versa.
There was a problem hiding this comment.
Okay, I thought about trying a simpler refactor, but if you say it is fine to change that to size_t, I'll do that later 😄
There was a problem hiding this comment.
Certainly beats changing our public API 😄
There was a problem hiding this comment.
Yeah, we have things that are not yet size_t but probably should be. They are mostly internal, though.
There was a problem hiding this comment.
I looked into this more today and I'm pretty sure changing numProp to size_t will not only change this field, but alot of other fields, structs, variables in methods and the public api too. I also found many other places where size_t and int are mismatched in parameter or variable types or int is used as vector index access where the vector size is type size_t.
My idea of going forward with this:
- I remove the numProp change from this PR
- We merge this PR with only the changes I mentioned in the issue
- I finish the rust bindings
- I come back to afterwards and try to rectify the
size_tproblem
There was a problem hiding this comment.
Sounds good, I think we can keep the internal things unchanged for now.
NickUfer
force-pushed
the
fix-size_t-int-mismatches
branch
from
af68c57 to
d9b5b75
Compare
NickUfer
force-pushed
the
fix-size_t-int-mismatches
branch
from
d9b5b75 to
dfb31b8
Compare
Fixes #1089