-
Notifications
You must be signed in to change notification settings - Fork 831
Avoid a few assertions that shouldn't be necessary #506
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
Conversation
EDIT: this is no longer the case, the API break is moved to #507 Since the change to |
@stevenroose Yeah, I'd prefer that. This way we can get a "fix" in 0.26 even before we properly fix it. |
525abbe
to
90e1125
Compare
K, I changed this PR to no longer change the API. Then I'll make another one for that. |
This is just a sanity check on our own serialization 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.
LGTM
FWIW most of these asserts (the size_of + bytes) will be optimized out in release builds anyway
Oh is that like a cargo optimization? I didn't know about that, but well this approach more explicitly expresses that intent, IMO. And perhaps other Rust compilers aren't that smart 😅 |
Can you add a commit which bumps the minor crate version and adds a changelog entry? I think this is the only thing we're targeting for 0.25.2. |
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.
ack 90e1125
e07ee51
Done. |
No, it's called "constant propogation", when the compiler see things that it can evaluate in compile time it will evaluate and change the branches accordingly, if 5< 3 {
println!("One");
} else {
println!("Two");
} Will const-propogate to: println!("Two"); so also: |
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.
re-ack e07ee51
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comme 8000 nt to others. Learn more.
ACK e07ee51
I went over all existing assertions, the ones I left are mostly wrong user input on methods that expect slices of a fixed size (like in decoding for endianness or so), I think those still make sense.
I just hit the
PublicKey::write_into
debug assertion running a long test involving programs sending messages over TCP connections (and not building with--release
). So when one peer closes a TCP connection at the exact moment the other side is writing a public key, the assertion would cause the sender to panic.