Skip to content

base64ct: extract and refactor with Encoding trait#238

Merged
tarcieri merged 1 commit intomasterfrom
trait-based-refactor
Feb 1, 2021
Merged

base64ct: extract and refactor with Encoding trait#238
tarcieri merged 1 commit intomasterfrom
trait-based-refactor

Conversation

@tarcieri
Copy link
Member

Refactors common encoding functionality into an Encoding trait, and defines each variant as a ZST which impls (or rather, receives a blanket impl) of the trait.

This makes trait-based abstractions around various encodings possible, and also eliminates a considerable amount of duplication.

@tarcieri tarcieri requested a review from newpavlov January 31, 2021 23:18
@tarcieri
Copy link
Member Author

Thought I'd try this approach before adding the final encoding I actually want (the crypt(3) encoding)

I'd like to allow password-hash to support multiple encodings besides unpadded Base64 (for sha-crypt specifically) but with a module-based API there's no way to do that.

@tarcieri tarcieri force-pushed the trait-based-refactor branch 4 times, most recently from 12e44a7 to 4afbe95 Compare February 1, 2021 00:39
Refactors common encoding functionality into an `Encoding` trait, and
defines each variant as a ZST which impls (or rather, receives a blanket
impl) of the trait.

This makes trait-based abstractions around various encodings possible,
and also eliminates a considerable amount of duplication.
@tarcieri tarcieri force-pushed the trait-based-refactor branch from 4afbe95 to 49cafa8 Compare February 1, 2021 01:07
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

I haven't gone over all changes, but on the first glance looks good!

The only suggestion I currently have is to merge Variant and Encoding traits into one and use default method implementations in terms of decode_6bits/encode_6bits. I think it should reduce code amount even further since you no longer need to define the second trait and write a blanket implementation for it.

Also I wonder if we even can go even further. Base64 variants can be generally described with 3-5 u8 ranges mapped into other u8 ranges, so we could in theory define a single constant with this mapping and implement the 6bits methods in terms of it. Though we would have to check if compiler properly inlines use of such constant.

@tarcieri
Copy link
Member Author

tarcieri commented Feb 1, 2021

The only suggestion I currently have is to merge Variant and Encoding traits into one

I was trying to avoid exposing this as part of the public API: Variant is a sealed trait.

...and use default method implementations in terms of decode_6bits/encode_6bits.

...but decode_6bits/encode_6bits is the only thing that's unique per encoding?

Base64 variants can be generally described with 3-5 u8 ranges mapped into other u8 ranges, so we could in theory define a single constant with this mapping and implement the 6bits methods in terms of it.

Since the purpose of this crate is to avoid LUTs, the only way to do this would be a sort of iterator over various range mapping operations to outputs... (or rather, two such iterators, one for encoding and one for decoding) however depending on circumstances those outputs may or may not include the source value itself (something we could capture in some sort of variant mapping, perhaps). It gets tricky enough I don't really feel an abstraction is helpful, or at least, I haven't found a good one yet.

But again, this is all the more reason to wrap this sort of thing up in a sealed trait. With a sealed trait we can experiment with these sorts of changes behind the scenes, whereas without one we'd be exposing it as part of the public API making any such internal refactoring a breaking change.

@tarcieri tarcieri merged commit 1a2dc28 into master Feb 1, 2021
@tarcieri tarcieri deleted the trait-based-refactor branch February 1, 2021 13:13
@newpavlov
Copy link
Member

newpavlov commented Feb 1, 2021

Ah, I missed the sealed part.

the only way to do this would be a sort of iterator over various range mapping operations to outputs

Yes, I thought about having an associated constant slice with arguments for match_range_ct/match_gt_ct. Assuming that loops over this constant will be unrolled, it should result in aroughly identical code.

This was referenced Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants