Generic Resource Identifiers with ResourceIdTrait
#90
Conversation
Benchmark for a882b55Click to view benchmark
|
|
Take a look at https://github.com/ARK-Builders/arklib/tree/blake3_ResourceIdTrait. It shows how easy it is to include a new |
src/resource/crc32.rs
Outdated
| fn get_hash(&self) -> Self::HashType { | ||
| self.hash | ||
| } |
There was a problem hiding this comment.
Not sure that we need to expose this. Probably ids should be "black boxes", i.e. users should only use them (via Eq, PartialEq, Ord, Display traits) without going into the details of how the ids are constructed.
There was a problem hiding this comment.
Currently, ResourceId has data_size and hash as public fields and I see them used in many places all over the codebase (especially in unit tests).
I thought if the hash is so frequently used, might be a good idea to add a method to get it.
What do you think?
There was a problem hiding this comment.
Let's consider this by use-cases:
- Unit tests should be able to access internal fields, but they should not be exposed to users/apps.
- Users/apps could use
data_sizebut if they need it, they would need it with other hash functions, too. It's better to provide such a field in some other way, generically for any hash function. Since we just pass this data to crc32 generation function, we could save it somewhere around. I don't think that users really need to do something withhashvalue, only with the id as a whole.
There was a problem hiding this comment.
Unit tests should be able to access internal fields
In the current setup, implementers of ResourceIdTrait have the freedom to decide whether to expose data_size and hash or not. It may be best not to enforce methods like get_hash in the trait itself.
Same goes for any methods related to data_size, considering that this field might not be applicable in all implementations of ResourceIdTrait, especially in cryptographic hash function implementations.
There was a problem hiding this comment.
We didn't include data_size in the draft for BLAKE3 implementation. Here #73 (comment)
Benchmark for 1e81c2bClick to view benchmark
|
Signed-off-by: Tarek <tareknaser360@gmail.com>
Signed-off-by: Tarek <tareknaser360@gmail.com>
Signed-off-by: Tarek <tareknaser360@gmail.com>
Signed-off-by: Tarek <tareknaser360@gmail.com>
Signed-off-by: Tarek <tareknaser360@gmail.com>
Signed-off-by: Tarek <tareknaser360@gmail.com>
Signed-off-by: Tarek <tareknaser360@gmail.com>
tareknaser
force-pushed
the
genericResourceId
branch
from
4617c88
to
447fbb3
Compare
Benchmark for d95e0a5Click to view benchmark
|
Description
This PR aims to make
ResourceIndextype generic over the hash function used to generate resource identifiersResolves #75
Changes
ResourceIdTrait. This trait defines a set of functions and requirements that all specificResourceIdimplementations must implementResourceIdTrait, a type alias namedHashTypeis defined. This allows differentResourceIdimplementations to use different types for their hash values, but we ensure that allHashTypetypes implement specific traits for consistent behaviorResourceIdCrc32type to implement theResourceIdTraitFuture work
src/index.rsfile heavily relies on the assumption of only usingResourceIdCrc32. It will require adjustments, potentially including abstracting some tests, to function with variousResourceIdtypes.ResourceIdBlake3that implementsResourceIdTraitsrc/resourcedirectory into a separate crate for better organization.