google / s2geometry

Computational geometry and spatial indexing on the sphere
http://s2geometry.io/
Apache License 2.0
2.29k stars 302 forks source link

Shouldn't this public function check for null? #292

Closed alas closed 1 year ago

alas commented 1 year ago

Shouldn't this public function check for null?

https://github.com/google/s2geometry/blob/ad5ef6c8cf4b0286868e3116e7347e2e114b4ff2/src/s2/s2lax_loop_shape.h#L76

context: https://github.com/google/s2geometry/blob/ad5ef6c8cf4b0286868e3116e7347e2e114b4ff2/src/s2/s2lax_loop_shape.h#L96

jmr commented 1 year ago

I don't think so. Could you explain why you think it should check for null?

The (undocumented, but hopefully apparent) precondition is 0 <= i && i < num_vertices(). Do you have an example test case that would be affected by your proposed change?

alas commented 1 year ago

I only asked cause it looks like a public facing API, I'm used to checking those for erroneous/malicious user input.

smcallis commented 1 year ago

This is a pretty common pattern in C++, in general you don't want to pay the cost for bounds checking. We have the new iteration API that prevents this is in the common case anyways. If we did bounds check then we have the question of what we would return when it fails. We don't use exceptions so we'd have to return a std::optional or absl::StatusOr probably, which would be a breaking change.

alas commented 1 year ago

closing then, ty