switch generic asn.1 der reader/writer to Io.Reader/Writer #25409
Conversation
|
cc @jedisct1 |
| var buffer: Io.Writer.Allocating = .init(self.allocator); | ||
| defer buffer.deinit(); | ||
| try buffer.writer.writeSplatAll(@constCast(data), splat); | ||
| self.prependSlice(buffer.written()) catch return error.WriteFailed; | ||
| return Io.Writer.countSplat(data, splat); |
There was a problem hiding this comment.
There's no need to double-allocate here. Increase capacity then copy the bytes into place.
There was a problem hiding this comment.
gotcha, I pushed a commit that does that instead.
lib/std/crypto/codecs/asn1.zig
Outdated
| }; | ||
|
|
||
| pub const DecodeError = error{ InvalidLength, EndOfStream }; | ||
| pub const DecodeError = error{InvalidLength} || std.Io.Reader.Error; |
There was a problem hiding this comment.
Incorrect error set - the fact that a reader is being used is an internal implementation detail that should not leak out here.
There was a problem hiding this comment.
ok, I made the errors set explicit (error{ InvalidLength, EndOfStream, ReadFailed }), is that preferable?
| /// Warning: This writer writes backwards. `fn print` will NOT work as expected. | ||
| pub fn writer(self: *Encoder) ArrayListReverse.Writer { | ||
| return self.buffer.writer(); | ||
| pub fn writer(self: *Encoder) *std.Io.Writer { |
There was a problem hiding this comment.
This is not your fault, but based on the doc comment for this function, it suggests this function should not exist.
There was a problem hiding this comment.
How about making it private? I can incorporate that into this PR if you'd like: e7097f8
| data: []u8, | ||
| capacity: usize, | ||
| allocator: Allocator, | ||
| writer: std.Io.Writer, |
There was a problem hiding this comment.
it's unclear to me that this should be how it works
There was a problem hiding this comment.
do you mean that the writer should not belong to the ArrayListReverse, or is it that an Io.Writer is not the right API here?
benburkert
force-pushed
the
asn1-reader-writer
branch
from
33eac1b to
946ed0d
Compare
|
thanks for the feedback @andrewrk, I tried to address what i could and left some additional questions. I didn't add full unit tests for the |
Followup to #25036 to finish moving the asn.1 der encoder & decoder to the
Io.Reader/Io.Writerinterfaces. It looks like the tests for those types were not being imported, so CI wasn't catching these compile errors.Fixes #25408