-
Notifications
You must be signed in to change notification settings - Fork 98
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
Type safety for indices #958
Conversation
LGTM |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #958 +/- ##
==========================================
- Coverage 91.84% 88.09% -3.75%
==========================================
Files 37 62 +25
Lines 4976 8685 +3709
Branches 0 1052 +1052
==========================================
+ Hits 4570 7651 +3081
- Misses 406 1034 +628 ☔ View full report in Codecov by Sentry. |
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 think this is probably a good idea, but can you show a little bit more, specifically how using these new indices looks? My main concern is code readability, so this will be okay so long as it's not adding lots of characters.
Also, should GetIndex
be a member function instead of a free function?
I don't think it will make code readability worse. I think perhaps some policy template could keep |
We do a lot of index addition/multiplication, so I just want to make sure that's as automatic with |
Gotcha. Well, we see how it looks :) |
I think this is just too hard a change to make, even if the result would be nice. |
Yeah, I'm not too surprised; thanks anyway. |
Manifold uses ints for indices which shouldn't be assigned to each other, such as vertex indices and half-edge indices. This proposal adds custom index support to
VecView
.This is a WIP, and I will attempt the rest of the changes with an OK from @elalish :)