switch impls based on crate features for u8 as an example #101
Conversation
|
@akhilles Any thoughts on this? |
|
Overall, LGTM. I think this is the right approach. |
Cargo.toml
Outdated
| @@ -14,6 +14,11 @@ keywords = ["crc", "crc16", "crc32", "crc64", "hash"] | |||
| categories = ["algorithms", "no-std"] | |||
| edition = "2018" | |||
|
|
|||
| [features] | |||
| notable-defaults = [] | |||
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I'll have another think about the naming
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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!
…us combinations of the features
Cargo.toml
Outdated
| @@ -14,6 +14,11 @@ keywords = ["crc", "crc16", "crc32", "crc64", "hash"] | |||
| categories = ["algorithms", "no-std"] | |||
| edition = "2018" | |||
|
|
|||
| [features] | |||
| notable-defaults = [] | |||
There was a problem hiding this comment.
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)
|
I don't think there is anything left to do, aside from agreeing on the naming for the features |
|
LGTM, thanks for your patience! |
As a draft towards #93, and #92
This introduces three crate features which enable choosing the default implementations. The implementation is chosen based on the flag with the most hardware compatibility ( => the highest restrictions on memory usage)