implement CidFromReader

[mvdan]

(see commit message)

[welcome[bot]]

Thank you for submitting this PR! A maintainer will be here shortly to review it. We are super grateful, but we are also overloaded! Help us by making sure that:

• The context for this PR is clear, with relevant discussion, decisions and stakeholders linked/mentioned.

• Your contribution itself is clear (code comments, self-review for the rest) and in its best form. Follow the code contribution guidelines if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment. Next steps:

• A maintainer will triage and assign priority to this PR, commenting on any missing things and potentially assigning a reviewer for high priority items.

• The PR gets reviews, discussed and approvals as needed.

• The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. We are very grateful for your contribution!

[mvdan]

Not super helpful that codecov is complaining about every uncovered line... I'd need to spend another 4h on this to get 100% coverage, and it feels somewhat futile.

[warpfork]

I'd love to have some tests that check that all the byte buffering dancing is correct. (It looks correct, but I've stopped trusting my eyes and mental model of the compiler completely in things as fiddly as this.)

• benchmarks are always good
• outright counting in a test is also possible -- https://github.com/ipld/go-ipld-prime/blob/b7347f196aa5653fddb5631124f8821ce425bfc9/codec/codectools/scratch/reader_test.go#L49 is an example of doing this

Either would be good. The latter is more fragile and gives less holistic impact numbers, but also results in something that can actually go red/green in CI rather than requiring someone to keep an eyeball on things, so both have different kinds of value.

I don't know if I consider this a blocking request or not, though. If it's already been profiled from some place where it's being used, I'll believe it's correct. Having tests or benchmarks in the same repo will just make it easier to be sure we don't ever regress it.

[warpfork]

Interface LGTM, anyway.

Returning an int like this to report partial progress in I/O reading is golang normative, yes. It describes a side effect that you couldn't otherwise get much insight into, and while that data's not always needed, when it is, you really need it, so it's good practice to always return it.

[mvdan]

I don't know if I consider this a blocking request or not, though. If it's already been profiled from some place where it's being used, I'll believe it's correct. Having tests or benchmarks in the same repo will just make it easier to be sure we don't ever regress it.

This PR is pure functionality, not optimization/performance. It's virtually impossible to accomplish this without CidFromReader today, unless you're happy with reading a "probably large enough" chunk of bytes and hoping for the best.

In that way, I don't think performance should block this PR. I agree benchmarks and profiling would be nice, but that seems like it should come after. All I can say on the matter is that our use in carv2 works, and seems fast enough :)

I admit I didn't go out of my way to write more tests to try to get 100% test coverage on this func. I'll see if I can reuse TestBadCidFromBytes for that purpose.

[mvdan]

The leading varint simplification that Steven suggested is now done.

I'll see if I can reuse TestBadCidFromBytes for that purpose.

That's now done too.

[mvdan]

@Stebalien the two byte slice comments are addressed, and the two perf nits now have TODOs. Want to take one last look?