Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add geo-traits crate #1157
Add geo-traits crate #1157
Changes from 39 commits
e337d8a
7f68df2
34ed5be
d57995e
7308df3
ee04201
ad48ee1
4261836
449b39f
f54fffc
db08c55
7ecdb7e
03f6615
31435e8
1d30066
ab0bf7b
33c1ef9
6363ad8
9421e05
9552705
3725844
7a2effd
8dcbe50
6889cae
de7ca8a
0fb5ab8
58117ce
6b697dd
2eb3b03
1f68cc3
b1364ef
d49873b
7c0e90f
09de278
8418780
4a9b3cc
ba9bf98
4764343
2b77691
711dfac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
When I first read this, I thought this would behave similarly to
Vec#get_unchecked
which is anunsafe
method which does not do a bounds check, hence it being marked asunsafe
. In fact, it looks like all functions named*_unchecked
Rust's standard library are marked asunsafe
, which to me implies they skip the bounds check.My first though was, what if we had a naming convention like this:
But that may psychologically push people to use the panic'ing version.
I don't know what the right answer is, just wanted to call out that I was surprised when implementing
CoordTrait
thatnth_unchecked
was not marked withunsafe
.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.
Another option 🤷🏻
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.
But we also definitely don't need to have an
unsafe
method, in fact probably better to leave it out for this initial implementationThere 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.
That's nice. 👏
We could also just leave out the
panic
flavor.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.
Although I didn't find it in the stdlib, apparently there's quite a bit of precedent for
_or_panic
in 3rd party crateshttps://github.com/search?q=or_panic+language%3ARust&type=code
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.
Open to suggestions here. I suppose it's worthwhile having a variant that is actually unsafe (e.g. doesn't even do the bounds check), for situations where you know you're in bounds (like the iterator)