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

Drop some unsafes - the compiler now optimizes equivalent safe code #43

Merged
merged 18 commits into from
Jun 26, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1a810db
Drop unsafe loops. On Rust 1.27 performance degradation is within mea…
Shnatsel Jun 22, 2018
488db57
Exit earlier on invalid run length, without leaving behind a buffer w…
Shnatsel Jun 24, 2018
a5e29c8
Fix formatting
Shnatsel Jun 24, 2018
bc3916d
Fix formatting in one more place
Shnatsel Jun 24, 2018
3715c4c
Add semicolon
Shnatsel Jun 24, 2018
11c73f2
run_len_dist: validate that dist is not 0. Fixes information disclosu…
Shnatsel Jun 25, 2018
8f22601
::std::intrinsics now requires 'core_intrinsics' feature, not 'core'
Shnatsel Jun 25, 2018
4e5cc94
Remove use of 'abort' intrinsic now that panic!() is optimized by the…
Shnatsel Jun 25, 2018
dd20d3e
Remove 'unstable' feature from Cargo.toml
Shnatsel Jun 25, 2018
d56abee
Replace panic!() with unreachable!(). Thank you @hauleth for the advice
Shnatsel Jun 25, 2018
61c2c1c
Revert "Remove 'unstable' feature from Cargo.toml"
Shnatsel Jun 26, 2018
d476136
Add an assert() before the last remaining unsafe {}; no performance p…
Shnatsel Jun 26, 2018
fdee4de
Add another assert against potentially reading from uninitialized memory
Shnatsel Jun 26, 2018
9c8f582
Add a comment explaining why `if dist < 1` check is there
Shnatsel Jun 26, 2018
a2955c8
Add an assert for dist correctness right before it's used. Surprising…
Shnatsel Jun 26, 2018
f4418ea
More comments warning the unwary about the one and only unsafe block
Shnatsel Jun 26, 2018
baf9c7e
Drop a wall of text in a comment, it is not helpful. There is an asse…
Shnatsel Jun 26, 2018
a36a517
Explain what each assert actually does
Shnatsel Jun 26, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ keywords = ["deflate", "decompression", "compression", "piston"]

[features]
default = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed at all? I think all it did was disable unstable by default.

unstable = []

[dependencies]
adler32 = "1.0.2"
28 changes: 4 additions & 24 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,6 @@
//! }
//! ```

#![cfg_attr(feature = "unstable", feature(core_intrinsics))]

extern crate adler32;

use std::cmp;
Expand Down Expand Up @@ -182,16 +180,6 @@ struct BitStream<'a> {
state: BitState,
}

// Use this instead of triggering a panic (that will unwind).
#[cfg(feature = "unstable")]
fn abort() -> ! {
unsafe { ::std::intrinsics::abort() }
}
#[cfg(not(feature = "unstable"))]
fn abort() -> ! {
panic!()
}

#[cfg(debug)]
macro_rules! debug { ($($x:tt)*) => (println!($($x)*)) }
#[cfg(not(debug))]
Expand Down Expand Up @@ -224,10 +212,7 @@ impl<'a> BitStream<'a> {
return false;
}
if n > 8 && self.state.n < n {
if n > 16 {
// HACK(eddyb) in place of a static assert.
abort();
}
assert!(n <= 16);
if !self.use_byte() {
return false;
}
Expand All @@ -248,10 +233,7 @@ impl<'a> BitStream<'a> {
}

fn take(&mut self, n: u8) -> Option<u8> {
if n > 8 {
// HACK(eddyb) in place of a static assert.
abort();
}
assert!(n <= 8);
self.take16(n).map(|v: u16| v as u8)
}

Expand Down Expand Up @@ -404,7 +386,7 @@ impl CodeLengthReader {
self.result.push(0);
}
}
_ => abort(),
_ => panic!(),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to use unreachable!() macro? It would provide more meaningful name.

}
}
Ok(true)
Expand Down Expand Up @@ -689,9 +671,7 @@ impl InflateStream {
if (self.pos as usize) < self.buffer.len() {
self.buffer[self.pos as usize] = b;
} else {
if (self.pos as usize) != self.buffer.len() {
abort();
}
assert_eq!(self.pos as usize, self.buffer.len());
self.buffer.push(b);
}
self.pos += 1;
Expand Down