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

switch impls based on crate features for u8 as an example #101

Merged
merged 13 commits into from
Apr 23, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
5 changes: 5 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ keywords = ["crc", "crc16", "crc32", "crc64", "hash"]
categories = ["algorithms", "no-std"]
edition = "2018"

[features]
notable-defaults = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] On naming, wdyt about "prefer-low-memory" and "prefer-fast" instead for "notable-defaults" and "slice16-defaults" respectively? They might be easier to understand for some users since it maps closer to intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd expect the people that care about any of this to also care about the specifics i.e. how much memory is "low memory" exactly at which point they also need to read about the different implementations. To then name the features after these implementations seems logical to me

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough, I still think the feature names can be a bit clearer though. Especially with "notable" as it can be interpreted as a single word. Maybe "prefer-no-table", "prefer-bytewise", "prefer-slice-by-16"? I think it's worth avoiding "default" as it's an overloaded term.

Also, would be good to add a comment on top of each feature (similar to https://github.com/serde-rs/serde/blob/master/serde/Cargo.toml#L39)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have another think about the naming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I came up with

  • no-table-memory-restrictions
  • bytewise-memory-restrictions
  • slice16-memory-restrictions

I think this captures the aspect of the additional restrictions one is imposing by activating the features, hopefully also conveying that those are additive.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, that's a good idea! I've also seen "limit" used in this context. Wdyt about using "mem-limit" or "memory-limit" as the suffix? Has the advantage that it's shorter. Sorry for being so picky on the naming :)

Copy link
Contributor Author

@KillingSpark KillingSpark Apr 23, 2023

Choose a reason for hiding this comment

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

Sorry for being so picky on the naming :)

No worries, naming is important and one of the hardest things to get right :)

"Limit" seems like a fine choice!

bytewise-defaults = []
akhilles marked this conversation as resolved.
Show resolved Hide resolved
slice16-defaults = []

[dependencies]
crc-catalog = "2.1.0"

Expand Down
61 changes: 58 additions & 3 deletions src/crc128.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use crc_catalog::Algorithm;

use crate::util::crc128;
use crc_catalog::Algorithm;

mod bytewise;
mod default;
Expand Down Expand Up @@ -170,9 +169,65 @@ const fn update_slice16(

#[cfg(test)]
mod test {
use crate::{Bytewise, Crc, NoTable, Slice16};
use crate::{Bytewise, Crc, Implementation, NoTable, Slice16};
use crc_catalog::{Algorithm, CRC_82_DARC};

#[test]
fn default_table_size() {
let table_size = core::mem::size_of::<<u128 as Implementation>::Table>();
akhilles marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(all(
feature = "notable-defaults",
feature = "bytewise-defaults",
feature = "slice16-defaults"
))]
assert_eq!(0, table_size);
#[cfg(all(
feature = "notable-defaults",
feature = "bytewise-defaults",
not(feature = "slice16-defaults")
))]
assert_eq!(0, table_size);
#[cfg(all(
feature = "notable-defaults",
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
assert_eq!(0, table_size);
#[cfg(all(
feature = "notable-defaults",
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
assert_eq!(0, table_size);

#[cfg(all(
not(feature = "notable-defaults"),
feature = "bytewise-defaults",
feature = "slice16-defaults"
))]
assert_eq!(256 * 16, table_size);
#[cfg(all(
not(feature = "notable-defaults"),
feature = "bytewise-defaults",
not(feature = "slice16-defaults")
))]
assert_eq!(256 * 16, table_size);

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
assert_eq!(256 * 16 * 16, table_size);

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
assert_eq!(256 * 16, table_size);
}

/// Test this opitimized version against the well known implementation to ensure correctness
#[test]
fn correctness() {
Expand Down
87 changes: 82 additions & 5 deletions src/crc128/default.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,62 @@
use crate::table::crc128_table;
use crate::{Algorithm, Crc, Digest};
use crate::crc128::{finalize, init};
use crate::{Algorithm, Crc, Digest, Implementation};

use super::{finalize, init, update_bytewise};
#[cfg(feature = "notable-defaults")]
impl Implementation for u128 {
type Width = u128;
type Table = ();
}

#[cfg(all(not(feature = "notable-defaults"), feature = "bytewise-defaults"))]
impl Implementation for u128 {
type Width = u128;
type Table = [u128; 256];
}

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
impl Implementation for u128 {
type Width = u128;
type Table = [[u128; 256]; 16];
}

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
impl Implementation for u128 {
type Width = u128;
type Table = [u128; 256];
}

impl Crc<u128> {
pub const fn new(algorithm: &'static Algorithm<u128>) -> Self {
akhilles marked this conversation as resolved.
Show resolved Hide resolved
let table = crc128_table(algorithm.width, algorithm.poly, algorithm.refin);
#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
let table =
crate::table::crc128_table_slice_16(algorithm.width, algorithm.poly, algorithm.refin);

#[cfg(all(not(feature = "notable-defaults"), feature = "bytewise-defaults"))]
let table = crate::table::crc128_table(algorithm.width, algorithm.poly, algorithm.refin);

#[cfg(feature = "notable-defaults")]
#[allow(clippy::let_unit_value)]
let table = ();

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
let table = crate::table::crc128_table(algorithm.width, algorithm.poly, algorithm.refin);

Self { algorithm, table }
}

Expand All @@ -16,7 +67,33 @@ impl Crc<u128> {
}

const fn update(&self, crc: u128, bytes: &[u8]) -> u128 {
update_bytewise(crc, self.algorithm.refin, &self.table, bytes)
#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
{
super::update_slice16(crc, self.algorithm.refin, &self.table, bytes)
}

#[cfg(all(not(feature = "notable-defaults"), feature = "bytewise-defaults"))]
{
super::update_bytewise(crc, self.algorithm.refin, &self.table, bytes)
}

#[cfg(feature = "notable-defaults")]
{
super::update_nolookup(crc, self.algorithm, bytes)
}

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
{
super::update_bytewise(crc, self.algorithm.refin, &self.table, bytes)
}
}

pub const fn digest(&self) -> Digest<u128> {
Expand Down
58 changes: 57 additions & 1 deletion src/crc16.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,65 @@ const fn update_slice16(

#[cfg(test)]
mod test {
use crate::{Bytewise, Crc, NoTable, Slice16};
use crate::{Bytewise, Crc, Implementation, NoTable, Slice16};
use crc_catalog::{Algorithm, CRC_16_IBM_SDLC};

#[test]
fn default_table_size() {
let table_size = core::mem::size_of::<<u16 as Implementation>::Table>();
#[cfg(all(
feature = "notable-defaults",
feature = "bytewise-defaults",
feature = "slice16-defaults"
))]
assert_eq!(0, table_size);
#[cfg(all(
feature = "notable-defaults",
feature = "bytewise-defaults",
not(feature = "slice16-defaults")
))]
assert_eq!(0, table_size);
#[cfg(all(
feature = "notable-defaults",
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
assert_eq!(0, table_size);
#[cfg(all(
feature = "notable-defaults",
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
assert_eq!(0, table_size);

#[cfg(all(
not(feature = "notable-defaults"),
feature = "bytewise-defaults",
feature = "slice16-defaults"
))]
assert_eq!(256 * 2, table_size);
#[cfg(all(
not(feature = "notable-defaults"),
feature = "bytewise-defaults",
not(feature = "slice16-defaults")
))]
assert_eq!(256 * 2, table_size);

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
assert_eq!(256 * 16 * 2, table_size);

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
assert_eq!(256 * 2, table_size);
}

/// Test this opitimized version against the well known implementation to ensure correctness
#[test]
fn correctness() {
Expand Down
86 changes: 81 additions & 5 deletions src/crc16/default.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,62 @@
use crate::crc16::{finalize, init};
use crate::table::crc16_table_slice_16;
use crate::{Algorithm, Crc, Digest};
use crate::{Algorithm, Crc, Digest, Implementation};

use super::update_slice16;
#[cfg(feature = "notable-defaults")]
impl Implementation for u16 {
type Width = u16;
type Table = ();
}

#[cfg(all(not(feature = "notable-defaults"), feature = "bytewise-defaults"))]
impl Implementation for u16 {
type Width = u16;
type Table = [u16; 256];
}

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
impl Implementation for u16 {
type Width = u16;
type Table = [[u16; 256]; 16];
}

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
impl Implementation for u16 {
type Width = u16;
type Table = [u16; 256];
}

impl Crc<u16> {
pub const fn new(algorithm: &'static Algorithm<u16>) -> Self {
let table = crc16_table_slice_16(algorithm.width, algorithm.poly, algorithm.refin);
#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
let table =
crate::table::crc16_table_slice_16(algorithm.width, algorithm.poly, algorithm.refin);

#[cfg(all(not(feature = "notable-defaults"), feature = "bytewise-defaults"))]
let table = crate::table::crc16_table(algorithm.width, algorithm.poly, algorithm.refin);

#[cfg(feature = "notable-defaults")]
#[allow(clippy::let_unit_value)]
let table = ();

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
let table = crate::table::crc16_table(algorithm.width, algorithm.poly, algorithm.refin);

Self { algorithm, table }
}

Expand All @@ -17,7 +67,33 @@ impl Crc<u16> {
}

const fn update(&self, crc: u16, bytes: &[u8]) -> u16 {
update_slice16(crc, self.algorithm.refin, &self.table, bytes)
#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
{
super::update_slice16(crc, self.algorithm.refin, &self.table, bytes)
}

#[cfg(all(not(feature = "notable-defaults"), feature = "bytewise-defaults"))]
{
super::update_bytewise(crc, self.algorithm.refin, &self.table, bytes)
}

#[cfg(feature = "notable-defaults")]
{
super::update_nolookup(crc, self.algorithm, bytes)
}

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
{
super::update_bytewise(crc, self.algorithm.refin, &self.table, bytes)
}
}

pub const fn digest(&self) -> Digest<u16> {
Expand Down
Loading