Use Go 1.18 generics in the store’s locking helpers

[mtrmac]

… avoiding quite a bit of the previously-required boilerplate.

This is probably easier to review as individual commits, and maybe with whitespace changes ignored.

@nalind PTAL

[giuseppe]

LGTM

[nalind]

Where did rcstore come from?

[mtrmac]

Thanks, fixed. (It’s a mistake in rebasing, rcstore used to exist back when this branch was originally prepared.)

[nalind]

Which err is this referring to, here and in various similar comments where the closure is calling Exists()?

[mtrmac]

The one that was assigned to _ when calling the locking wrapper…

It would be possible to explain that that in those comments, but at that point it seems simpler to just make the code explicit if err { return false }; if found { return true } now. So updated that way.

[nalind]

It is, thanks. Generally looks good, though I'm mildly surprised that it doesn't make the logic that much shorter.
Defining a package-local void type when struct{} is widely used just makes me do a double-take when I read it.

[mtrmac]

I'm mildly surprised that it doesn't make the logic that much shorter.

I’m afraid that’s just Go. Native error handling and more type inference would be necessary to significantly shorten the boilerplate.

Another modern language:

class Store {
    var containerStore: ContainerStore = ContainerStore()
    
    private func readContainerStore<T>(_ fn: () throws -> T) throws -> T {
        try containerStore.startReading()
        defer {
            containerStore.stopReading()
        }
        return try fn()
    }
    
    func containerBigData(id: String, key: String) throws -> [UInt8] {
        return try readContainerStore {
            return try containerStore.bigData(id: id, key: key)
        }
    }
}

(And actually, with that kind of a language with a working private, we could compile-time enforce that only lock holders can perform operations.

The grass and the other side…)

---

Defining a package-local void type when struct{} is widely used just makes me do a double-take when I read it.

I chose this because I thought the slight readability benefit, making it very clear that there is nothing there, is worth it — and because I didn’t want to keep typing that struct{}{} — but that’s a very weakly held aesthetic opinion.

I fully realize that this is a fairly goofy approach, and I’m perfectly willing to drop that part.

[mtrmac]

I chose this because I thought the slight readability benefit, making it very clear that there is nothing there, is worth it

(Another possible alternative would be to define another set of generic wrappers which don’t simply don’t have that return value, e.g.

func writeToLayerStoreVoid(s *store, fn func(store rwLayerStore) (error)) (error) {
    return writeToLayerStore(s, func(store rwLayerStore) (struct{}, error) {
        return struct{}{}, fn(store)
    })
}

but that feels tedious to me.)

[mtrmac]

Updated: I have changed my mind on readContainerStore returning an extra bool. In most callers it causes extra boilerplate, but a few really do need to distinguish between a locking failure, item not found, and valid data. Also the symmetry with the other two read* helpers is a bit easier to follow in the more complex callers.

[nalind]

Defining a package-local void type when struct{} is widely used just makes me do a double-take when I read it.

I chose this because I thought the slight readability benefit, making it very clear that there is nothing there, is worth it — and because I didn’t want to keep typing that struct{}{} — but that’s a very weakly held aesthetic opinion.

I fully realize that this is a fairly goofy approach, and I’m perfectly willing to drop that part.

Yes, please go ahead and drop that part. I don't have an issue with returning an unused variable from a generic convenience function.

[mtrmac]

void dropped.

[nalind]

If I'm reading it right, err is always nil here.

[mtrmac]

err can be set if readAllImageStores fails in imageStore.startReading, e.g. on an I/O error.

[nalind]

One nit in ImageBigDataSize, otherwise LGTM.

[rhatdan]

LGTM