Skip to content

stdlib : base64 encode to writer#20961

Merged
jedisct1 merged 6 commits intoziglang:masterfrom
Arwalk:add_stream_writer_base64
Sep 4, 2024
Merged

stdlib : base64 encode to writer#20961
jedisct1 merged 6 commits intoziglang:masterfrom
Arwalk:add_stream_writer_base64

Conversation

@Arwalk
Copy link
Contributor

@Arwalk Arwalk commented Aug 6, 2024

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 Arwalk force-pushed the add_stream_writer_base64 branch from 750cfea to db0c706 Compare August 6, 2024 20:11
@jedisct1
Copy link
Contributor

jedisct1 commented Aug 6, 2024

This is a useful addition, thank you!

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

@Arwalk
Copy link
Contributor Author

Arwalk commented Aug 6, 2024

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
Copy link
Contributor Author

Arwalk commented Aug 6, 2024

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
Copy link
Contributor Author

Arwalk commented Aug 6, 2024

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

@Arwalk
Copy link
Contributor Author

Arwalk commented Aug 8, 2024

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 ab740ca

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

@Arwalk
Copy link
Contributor Author

Arwalk commented Aug 8, 2024

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

@jedisct1
Copy link
Contributor

jedisct1 commented Aug 8, 2024

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.

@Arwalk
Copy link
Contributor Author

Arwalk commented Aug 9, 2024

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 Arwalk force-pushed the add_stream_writer_base64 branch from ab740ca to ffc5b08 Compare August 13, 2024 16:30
@Arwalk
Copy link
Contributor Author

Arwalk commented Aug 13, 2024

@marler8997 i applied your suggestions to the other method in b9dfe0f

@Arwalk Arwalk force-pushed the add_stream_writer_base64 branch from b9dfe0f to 51c2b14 Compare August 14, 2024 15:09
@Arwalk
Copy link
Contributor Author

Arwalk commented Aug 14, 2024

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

@Arwalk
Copy link
Contributor Author

Arwalk commented Aug 29, 2024

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

@Vexu Vexu enabled auto-merge (squash) September 1, 2024 14:27
auto-merge was automatically disabled September 3, 2024 22:56

Head branch was pushed to by a user without write access

@Arwalk Arwalk force-pushed the add_stream_writer_base64 branch from 51c2b14 to bcf4d78 Compare September 3, 2024 22:56
@Arwalk
Copy link
Contributor Author

Arwalk commented Sep 3, 2024

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.

@jedisct1 jedisct1 enabled auto-merge (squash) September 4, 2024 04:53
@jedisct1 jedisct1 merged commit f87dd43 into ziglang:master Sep 4, 2024
@Arwalk Arwalk deleted the add_stream_writer_base64 branch September 4, 2024 13:26
richerfu pushed a commit to richerfu/zig that referenced this pull request Oct 28, 2024
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.

3 participants