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

Use new_checked by default #195

Closed
dlrobertson opened this issue Apr 20, 2018 · 2 comments
Closed

Use new_checked by default #195

dlrobertson opened this issue Apr 20, 2018 · 2 comments

Comments

@dlrobertson
Copy link
Collaborator

Currently all Packet types have at least the two constructor methods new and new_checked. new does no length checks, while new_checked obviously does. AFAIK in std etc typically the "safe" version is the default and the "unsafe" version is explicitly called with <name>_unchecked. Would it make sense to do something like the following?

  1. deprecate Packet::new_checked
  2. make Packet::new functions do what Packet::new_checked does
  3. add Packet::new_unchecked functions that do what Packet::new does
@dlrobertson
Copy link
Collaborator Author

Just a question/thought

@whitequark
Copy link
Contributor

There is nothing inherently safer about constructing packets with new_checked because you can still get panics if you modify a length field inside the packet and then use an accessor.

Moreover, it's not even that one should be preferred over the other. In a typical network stack that both sends and receives packets there would be approximately as many calls to new as to new_checked.

I'm open to other suggestions for these names, but I think that safety is the wrong angle here.

m-labs-homu pushed a commit that referenced this issue Jul 11, 2018
m-labs-homu pushed a commit that referenced this issue Jul 11, 2018
m-labs-homu pushed a commit that referenced this issue Jul 11, 2018
m-labs-homu pushed a commit that referenced this issue Jul 11, 2018
Fixes #195.

Closes: #254
Approved by: dlrobertson
m-labs-homu pushed a commit that referenced this issue Jul 11, 2018
Fixes #195.

Closes: #254
Approved by: dlrobertson
m-labs-homu pushed a commit that referenced this issue Jul 11, 2018
Fixes #195.

Closes: #254
Approved by: dlrobertson
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants