New Reader convenience methods for reading into {int, uint8, uint16, uint32} without caller type conversion.

[jpap]

These methods significantly simplified my code, when parsing a bitstream into a struct.

It extends the API, and does not modify existing method signatures.

[codecov-io]

Codecov Report

Merging #3 into master will decrease coverage by 16.75%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           master       #3       +/-   ##
===========================================
- Coverage     100%   83.24%   -16.76%     
===========================================
  Files           2        2               
  Lines         149      179       +30     
===========================================
  Hits          149      149               
- Misses          0       30       +30

Impacted Files	Coverage Δ
reader.go	70.29% <0%> (-29.71%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b25b30b...8c0a7be. Read the comment docs.

[icza]

Size of int is platform specific, so I wouldn't include ReadBitsAsInt() in it because a call might succeed on a 64-bit platform and fail on a 32-bit platform. Clients should be explicit and use uint32.

I see the uses of ReadBitsAsUInt8(), ReadBitsAsUInt16() and ReadBitsAsUInt32(), but I would see more use if they would not just call ReadBits() which operates on a 64-bit value, but would implement working with an 8-bit, 16-bit or 32-bit value inside, to be more efficient.

Also if we're adding varying sizes for reads, why not do the same for writes?

Therefore I tend not to include this PR. ReadBitsAsUInt8(), ReadBitsAsUInt16() and ReadBitsAsUInt32() could simply exist as utility functions.

[jpap]

Size of int is platform specific, so I wouldn't include ReadBitsAsInt() in it because a call might succeed on a 64-bit platform and fail on a 32-bit platform. Clients should be explicit and use uint32.

How about limiting ReadBitsAsInt to 32-bits? (This method is included as a convenience; it does not have to cater to all use-cases and works perfectly well for my purposes with this limitation.)

I see the uses of ReadBitsAsUInt8(), ReadBitsAsUInt16() and ReadBitsAsUInt32(), but I would see more use if they would not just call ReadBits() which operates on a 64-bit value, but would implement working with an 8-bit, 16-bit or 32-bit value inside, to be more efficient.

Yes, that's feasible. Please LMK if you're open to this in an amended PR.

Also if we're adding varying sizes for reads, why not do the same for writes?

There is less incentive to do so for writes, as it's very easy to typecast to uint64 when invoking a write, using a one-liner.

The problem with the ReadXXX methods is the multiple return values; you can't simply typecast the uint64 value because of the error value: the call ends up being several lines, as you can see in the proposed ReadBitsAsXXX methods in this PR.

Therefore I tend not to include this PR. ReadBitsAsUInt8(), ReadBitsAsUInt16() and ReadBitsAsUInt32() could simply exist as utility functions.

Of course they can; but it is significantly more convenient for these methods to be built into the package: the user doesn't have to write them over-and-over, whenever they use the package.

In my case, I use bitio in several packages within the same project which means either: A) I'd have to copy the helpers in each, B) create a separate helper package just for this, or C) fork the bitio project (/sigh/).

[icza]

The thing is there are also signed types, e.g. int, int8, int16, int32, int64. If the user has these, he still needs utility functions because ReadBitsAsUInt16() returns uint16 and error, and that still can't be converted in a single line. You may not have a need for this, but somebody else might.

Adding all signed and unsigned variants would bloat the API.

So I propose an alternative: MustXXX() methods.

Add a new field to reader: mustErr error

func (r *reader) MustReadBits(n byte) (u uint64) {
    // If there was a previous error, do nothing:
    if r.mustErr = nil {
        u, r.mustErr = r.ReadBits(n)
    }
    return
}

Now you can do a one-liner just like with Writer:

var i int32
var j uint32
var r Reader

i = int32(r.MustReadBits(20))
j = uint32(r.MustReadBits(20))
// You can check error like this, and it's "enough" to do it once:
if r.MustErr() != nil {
    // Handle error
}

This way you don't have to add all variants, and you can even defer error checking, reading all the fields you need and just check error once.

If you like this and would satisfy you, I'd rather implement this.

[jpap]

Great idea! That would significantly reduce boilerplate even further.

I would only suggest we refine the naming. Where the Go standard library uses the word "Must", it usually accompanies a panic on error. Also, the word "Error" isn't usually abbreviated; for example, see the error interface definition itself:

type error interface {
    Error() string
}

In this case, please consider using a softer name like: TryReadBits (TryReadingBits?).
Returning the error condition might then be LastError() error, or if you want to limit yourself to reads, then LastReadError() error.

[icza]

You're right on the naming conventions, it was just a quick idea, not yet refined.

TryReadBits() looks good, and will use TryError() to get the TryXXX() error result.

[icza]

I also wish to remove Reader and Writer interfaces. The reader and writer structs should be exported and returned, this is the convention in Go (return concrete types, expect interfaces). This may be a breaking change for some, but I'd wish to make this step. The package is not yet versioned, after this it could enter v1.0.0. What do you think?

[icza]

OK, it's done. I added TryXXX() methods to both Reader and Writer. I also updated the documentation and README. Closing this PR.

[jpap]

Amazing work. Thank you for taking the time on this issue and your efforts over the past couple of years maintaining bitio -- it's a great library.

I also wish to remove Reader and Writer interfaces. The reader and writer structs should be exported and returned, this is the convention in Go (return concrete types, expect interfaces). This may be a breaking change for some, but I'd wish to make this step. The package is not yet versioned, after this it could enter v1.0.0. What do you think?

I like it -- simplifies it further.

OK, it's done. I added TryXXX() methods to both Reader and Writer. I also updated the documentation and README. Closing this PR.

Excellent! I also like how TryError is a field variable; it allows us to clear it as we (might) need, whereas the func version we discussed is more limited.