Skip to content
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

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

Closed
alas opened this issue Dec 30, 2022 · 4 comments
Closed

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

alas opened this issue Dec 30, 2022 · 4 comments

Comments

@alas
Copy link
Contributor

alas commented Dec 30, 2022

Shouldn't this public function check for null?

const S2Point& vertex(int i) const { return vertices_[i]; }

context:

std::unique_ptr<S2Point[]> vertices_;

@jmr
Copy link
Member

jmr commented Dec 30, 2022

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
Copy link
Contributor Author

alas commented Dec 30, 2022

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

@smcallis
Copy link
Collaborator

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
Copy link
Contributor Author

alas commented Dec 30, 2022

closing then, ty

@jmr jmr closed this as completed Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants