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

SliceSeekableStream #1815

Closed
ghost opened this issue Dec 3, 2018 · 8 comments
Closed

SliceSeekableStream #1815

ghost opened this issue Dec 3, 2018 · 8 comments
Labels
standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@ghost
Copy link

ghost commented Dec 3, 2018

I noticed that SeekableStream has been added to the zig std, which is nice. But it only seems to be useable with FileInStream currently. I get a lot of use out of SliceInStream so I was pondering how to get it working with that (then I can get rid of a lot of code in my personal zigutils library). So I propose a somewhat drastic change to SliceInStream - a new object type at the heart:

// this needs a better name
pub const SliceWithCursor = struct{
    slice: []const u8,
    pos: usize,
};

This is analogous to the File object for file streams. SliceInStream (and SliceOutStream?) holds a pointer to one of these, rather than the slice directly. Now you can easily implement SliceSeekableStream as well.

Downside: using these things becomes even more verbose. We end up with this:

var swc = SliceWithCursor.init("this is a slice");
var in_stream = SliceInStream.init(&swc);
var seekable = SliceSeekableStream.init(&swc);

// this could be done with `var` params as well, which isn't great either
SomethingThatNeedsASeekableInStream(
    SliceInStream.Error,
    SliceSeekableStream.SeekError,
    SliceSeekableStream.GetSeekPosError,
).doSomething(&in_stream.stream, &seekable.stream);

Full code:

https://github.com/dbandstra/zigutils/blob/master/src/SliceStream.zig

Aside: Maybe SliceInStream, SliceOutStream, and SliceSeekableStream could be placed inside the SliceWithCursor struct namespace, like it is in the file code. I'm not personally a fan of that but Zig seems to go that way.

@tgschultz
Copy link
Contributor

tgschultz commented Dec 3, 2018

I don't understand. What is it about SliceInStream and SliceOutStream that requires wrapping then in another struct?

Here, I took your code and just removed the SliceWithCursor and replaced it with SliceInStream and it works fine:

pub const SliceSeekableStream = struct{
  const Self = @This();
  pub const SeekError = error{SliceSeekOutOfBounds};
  pub const GetSeekPosError = error{};
  pub const Stream = std.io.SeekableStream(SeekError, GetSeekPosError);

  pub stream: Stream,

  swc: *std.io.SliceInStream,

  pub fn init(swc: *std.io.SliceInStream) Self {
    return Self{
      .stream = Stream{
        .seekToFn = seekToFn,
        .seekForwardFn = seekForwardFn,
        .getEndPosFn = getEndPosFn,
        .getPosFn = getPosFn,
      },
      .swc = swc,
    };
  }

  fn seekToFn(seekable_stream: *Stream, pos: usize) SeekError!void {
    const self = @fieldParentPtr(Self, "stream", seekable_stream);

    // FIXME - are you supposed to be able to seek past the end without an error?
    // i suppose it would only fail if you tried to read at that point?

    if (pos < self.swc.slice.len) {
      self.swc.pos = pos;
    } else {
      return SeekError.SliceSeekOutOfBounds;
    }
  }

  fn seekForwardFn(seekable_stream: *Stream, amt: isize) SeekError!void {
    const self = @fieldParentPtr(Self, "stream", seekable_stream);

    if (amt > 0) {
      const uofs = @intCast(usize, amt); // should never fail

      if (self.swc.pos + uofs <= self.swc.slice.len) {
        self.swc.pos += uofs;
      } else {
        return SeekError.SliceSeekOutOfBounds;
      }
    } else if (amt < 0) {
      const uofs = @intCast(usize, -amt); // should never fail

      if (self.swc.pos >= uofs) {
        self.swc.pos -= uofs;
      } else {
        return SeekError.SliceSeekOutOfBounds;
      }
    }
  }

  fn getEndPosFn(seekable_stream: *Stream) GetSeekPosError!usize {
    const self = @fieldParentPtr(Self, "stream", seekable_stream);

    return self.swc.slice.len;
  }

  fn getPosFn(seekable_stream: *Stream) GetSeekPosError!usize {
    const self = @fieldParentPtr(Self, "stream", seekable_stream);

    return self.swc.pos;
  }
};

// Removed now irrelevant tests

test "SliceStream: seeking around" {
  var sis = std.io.SliceInStream.init("This is a decently long sentence.");
  var sss = SliceSeekableStream.init(&sis);

  var dest_buf: [5]u8 = undefined;

  const br0 = try sis.stream.read(dest_buf[0..]);
  std.debug.assert(std.mem.eql(u8, dest_buf[0..br0], "This "));

  try sss.stream.seekForward(3);
  const br1 = try sis.stream.read(dest_buf[0..]);
  std.debug.assert(std.mem.eql(u8, dest_buf[0..br1], "a dec"));

  try sss.stream.seekForward(-2);
  const br2 = try sis.stream.read(dest_buf[0..]);
  std.debug.assert(std.mem.eql(u8, dest_buf[0..br2], "ecent"));

  try sss.stream.seekForward(0);
  const cur_pos = try sss.stream.getPos();
  std.debug.assert(cur_pos == 16);

  try sss.stream.seekTo(1);
  const br3 = try sis.stream.read(dest_buf[0..]);
  std.debug.assert(std.mem.eql(u8, dest_buf[0..br3], "his i"));

  try sss.stream.seekTo(sis.slice.len - 3);
  const br4 = try sis.stream.read(dest_buf[0..]);
  std.debug.assert(std.mem.eql(u8, dest_buf[0..br4], "ce."));

  std.debug.assertError(sss.stream.seekTo(999), error.SliceSeekOutOfBounds);
  std.debug.assertError(sss.stream.seekForward(-999), error.SliceSeekOutOfBounds);
}

@andrewrk andrewrk added this to the 0.4.0 milestone Dec 3, 2018
@ghost
Copy link
Author

ghost commented Dec 5, 2018

What about SliceOutStream though? (allowing both seekable in streams and seekable out streams)

@tgschultz
Copy link
Contributor

tgschultz commented Dec 5, 2018

What about it? As far as I can tell it should work just fine with a few changes:

pub fn SliceSeekableStream(comptime SliceStream: type) type {
    return struct {
      const Self = @This();
      pub const SeekError = error{SliceSeekOutOfBounds};
      pub const GetSeekPosError = error{};
      pub const Stream = std.io.SeekableStream(SeekError, GetSeekPosError);

     pub stream: Stream,

     swc: *SliceStream,
     ....

@ghost
Copy link
Author

ghost commented Dec 5, 2018

I wish we didn't have to use ad hoc unwritten interfaces but ok. We are here now:

var in_stream = std.io.SliceInStream.init("this is a slice");
var seekable = SliceSeekableStream(std.io.SliceInStream).init(&in_stream);

SomethingThatNeedsASeekableInStream(
    std.io.SliceInStream.Error,
    SliceSeekableStream(std.io.SliceInStream).SeekError,
    SliceSeekableStream(std.io.SliceInStream).GetSeekPosError,
).doSomething(&in_stream.stream, &seekable.stream);

Could maybe move those error types out of the seekable struct, or just use vars instead of having to pass them explicitly.

I suppose this is treading on other proposals about traits, constrained generic types etc. I don't have anything to add to those discussions since I'm not really up to speed. I'm not sure what the Zig idiom is / where it's heading.

@tgschultz
Copy link
Contributor

tgschultz commented Dec 5, 2018

Well they could be predefined in the standard lib:

const SeekableSliceInStream = SliceSeekableStream(SliceInStream);
const SeekableSliceOutStream = SliceSeekableStream(SliceOutStream);

Passing the Errors around like this are an unfortunate consequence of how streams are done currently in std. They're written as runtime interfaces but are effectively comptime because of the ErrorSet information. I do think that's a separate, but important, issue to address though.

See: #764

@ghost
Copy link
Author

ghost commented Dec 14, 2018

Another flaw with my idea - SliceInStream uses a []const u8 slice, but SliceOutStream uses a []u8 slice. So even the SliceWithCursor object would have to be made generic.

I'm trying out some different things now, in light of #1829. My current idea is two structs, IConstSlice and ISlice (again, naming could be better).

  • IConstSlice implements InStream and SeekableStream (and has methods to instantiate them)
  • ISlice implements OutStream and SeekableStream (ditto)

There's also IFile which implements all three stream types. (That ought to be baked in to std.os.File itself, but I think the File param not being a pointer breaks the generic code.)

Anyways, code here: https://github.com/dbandstra/zigutils/tree/new-streams/src/streams

Unfortunately, the SeekableStream implementations are copy-pasted between IConstSlice and ISlice. But on the positive side, this idea minimizes needless indirections.

@andrewrk andrewrk added the standard library This issue involves writing Zig code for the standard library. label Feb 10, 2019
@andrewrk andrewrk modified the milestones: 0.4.0, 0.5.0 Feb 10, 2019
@ghost
Copy link
Author

ghost commented Mar 10, 2019

I'm going to close this, I don't have the interest to follow it up. It seems like other people have a better handle on the whole trait thing, so this will probably get improved as part of a larger sweeping change anyway.

@ghost ghost closed this as completed Mar 10, 2019
@andrewrk
Copy link
Member

so this will probably get improved as part of a larger sweeping change anyway.

👍 you're not alone; lots of us agree this is a problem, and there are some other issues open to track this.

@andrewrk andrewrk modified the milestones: 0.5.0, 0.4.0 Apr 8, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

No branches or pull requests

2 participants