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.89% +0.01%
==========================================
Files 30 30
Lines 5882 5882
==========================================
+ Hits 5404 5405 +1
+ Misses 478 477 -1 ☔ View full report in Codecov by Sentry. |
NickUfer
force-pushed
the
fix-size_t-int-mismatches
branch
from
6a8db7b
to
af92593
Compare
refactor(1089): change NumProp getter type to int
NickUfer
force-pushed
the
fix-size_t-int-mismatches
branch
from
af92593
to
af68c57
Compare
| @@ -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
Fixes #1089