RFC: Use Go 1.23 iterators for locking+traversing stores.#2288
RFC: Use Go 1.23 iterators for locking+traversing stores.#2288mtrmac wants to merge 1 commit intocontainers:mainfrom
Conversation
This replaces one kind of boilerplate with another, but overall seems a bit less repetitive. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtrmac The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Luap99
left a comment
There was a problem hiding this comment.
I am not an active c/storage contributor but I read the code regularly (i.e. when debugging a podman issue) and this seems to me clearly an improvement over the callback. It feels much more natural to read the callers as loops so I like this a lot.
| // … | ||
| // }; done { | ||
| // return res, err | ||
| // for rlstore, err := range s.readAllLayerStores() { |
There was a problem hiding this comment.
Note to self: This looks almost too easy, without warning callers about the semantics. Maybe range iterateLockingAllLayerStores ?
There was a problem hiding this comment.
iterateReadLockingAllLayerStores? As usual, I tend towards unwieldy names.
|
Hi, and thank you for your contribution! We’ve recently migrated this repository into a new monorepo: containers/container-libs along with other repositories As part of this migration, this repository is no longer accepting new Pull-Requests and therefore this Pull-Request is being closed. Thank you very much for your contribution. We would appreciate your continued help in migrating this PR to the new container-libs repository. Please let us know if you are facing any issues. You can read more about the migration and the reasoning behind it in our blog post: Upcoming migration of three containers repositories to monorepo. Thanks again for your work and for supporting the containers ecosystem! |
RFC: this is a ~demo, only migrating one function out of several.
This replaces one kind of boilerplate with another, but overall seems a bit less repetitive. The primary benefit is that the code working within the locked stores can
returndirectly, without boilerplate, and there is no need to manually declare afunc(…)for the loop body.The
for store, err := rangesyntax is a bit weird, admittedly, but maybe worth it.The primary downside is that this really doesn’t work for the exactly-one-closure-call helpers (
writeTo…Store):for store, err := range writeToSingleStoreis very unintuitive, but maybe acceptable with good helper namesreturns, there is no need for the code afterwards toreturnas well. So every caller would need to add an unreachable inventedreturnthere… and that seems a bit much for me.So, we would end up with two rather different helper patterns instead of ~one. I’m really unsure whether it is worth it. It might well make sense to leave the code as is, and to wait another 1/5/10 years for Go to gain type inference and a convenient closure syntax.