zig icon indicating copy to clipboard operation
zig copied to clipboard

stdlib : base64 encode to writer

Open Arwalk opened this issue 1 year ago • 8 comments

Hello,

The current base64 interface only allows encoding to slices, making it impractical (although not impossible) to use when encoding dynamically to a document (such as a json string, as an example).

This PR adds the encodeWriter API to Base64Encoder, relying on the destination having the writeAll interface of Writer. As it is possible to encode base64 data by chunks of 3 bytes, it progressively encodes the source data in the stream without unnecessary heap allocations.

Arwalk avatar Aug 06 '24 19:08 Arwalk

This is a useful addition, thank you!

Ideally, a reader interface would also be great, but this is much more complicated.

jedisct1 avatar Aug 06 '24 20:08 jedisct1

Ideally, a reader interface would also be great, but this is much more complicated.

If i'm motivated i'll take a look into it. I think if we base ourserves on Reader's readBoundedBytes we should be able to chunk the decoding there too?

Arwalk avatar Aug 06 '24 21:08 Arwalk

I realize i removed the comptime keyword on a test that was explicitely comptime. I should try to find a way to avoid this.

Arwalk avatar Aug 06 '24 21:08 Arwalk

There, using BoundedArray the tests works at comptime, while still showing that it uses Writer's writeAll.

Arwalk avatar Aug 06 '24 21:08 Arwalk

Ideally, a reader interface would also be great, but this is much more complicated.

So i looked into it for a bit, and it's actually not that hard. The Reader interface is pretty neat and allows us to do this quite easily, so i added this interface in ab740ca2d43b889ff48efbaebe6eb686b7e87e84

This means in theory that you could encode a file directly to base64 without having to load it in memory. Sounds pretty nice !

Arwalk avatar Aug 08 '24 05:08 Arwalk

while i'm at it, should i try to do the reverse for decoding ?

Arwalk avatar Aug 08 '24 06:08 Arwalk

while i'm at it, should i try to do the reverse for decoding ?

That could be great, if only for consistency. But in a distinct PR.

jedisct1 avatar Aug 08 '24 10:08 jedisct1

Is there anything missing for this PR @jedisct1 ? Not trying to stress you on the matter, genuinely asking, as I do not know if there are any other prerequisites.

Thank you for your feedback.

Arwalk avatar Aug 09 '24 08:08 Arwalk

@marler8997 i applied your suggestions to the other method in https://github.com/ziglang/zig/pull/20961/commits/b9dfe0f1b9aaa7a8744c8400d578b36cbc197865

Arwalk avatar Aug 13 '24 17:08 Arwalk

Now with a zig fmt pass that was missing, sorry.

Arwalk avatar Aug 14 '24 15:08 Arwalk

Hey there. Anything missing here so it can be merged? I'm planning on doing the decoding part soon in another PR.

Arwalk avatar Aug 29 '24 19:08 Arwalk

thanks @Vexu for the auto-merge. Could you reenable it? I just rebased again. There was an error in aarch-linux-release, but it wasn't related to my stuff.

Arwalk avatar Sep 03 '24 22:09 Arwalk