Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stdlib : base64 encode to writer #20961

Merged
merged 6 commits into from
Sep 4, 2024

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.

@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.

lib/std/base64.zig Outdated Show resolved Hide resolved
@Arwalk
Copy link
Contributor Author

Arwalk commented Aug 13, 2024

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

@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
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
10 checks passed
@Arwalk Arwalk deleted the add_stream_writer_base64 branch September 4, 2024 13:26
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