Add convenience func to reverse callbacks and refactor other methods #7142
Conversation
chatton
requested review from
colin-axner,
AdityaSripal,
damiannolan and
srdtrk
as code owners
| negotiatedVersions = slices.Clone(negotiatedVersions) | ||
| slices.Reverse(negotiatedVersions) |
There was a problem hiding this comment.
this is the one thing I don't like about this PR, the negotiatedVersions are provided in reverse order.
I think we can either do this, require the caller to provide the calling fn to reverse them (easy to forget), or just reverse directly without copying, which will likely not be a problem but this version is just being extra careful.
There was a problem hiding this comment.
Hmm, yeah. Not beautiful, but its all part of this legacy module construction so I'm fine with it
|
| negotiatedVersions := make([]string, len(im.cbs)) | ||
|
|
||
| for i := len(im.cbs) - 1; i >= 0; i-- { | ||
| var negotiatedVersions []string |
There was a problem hiding this comment.
[nit] we could be slightly more efficient by doing:
negotiatedVersions := make([]string,0, len(im.cbs)) which would pre-allocate the needed capacity (but not the length).
If we know there's a 1:1 relationship between im.cbs and negotiatedVersions (so that the length is exactly the same) we could even do
negotiatedVersions := make([]string,len(im.cbs)
for i, cb := range im.reverseCallbacks() {
...
negotiatedversions[i] =| // reverseCallbacks returns a copy of the callbacks in reverse order. | ||
| // the majority of handlers are called in reverse order, so this can be used | ||
| // in those cases to prevent needing to iterate backwards over the callbacks. | ||
| func (im *LegacyIBCModule) reverseCallbacks() []ClassicIBCModule { |
There was a problem hiding this comment.
[nit] since this doesn't actually reverse the callbacks, but returns the "reversed", what about calling it reversedCallbacks?
There was a problem hiding this comment.
sgtm, I can't think of any better naming tbh
| negotiatedVersions = slices.Clone(negotiatedVersions) | ||
| slices.Reverse(negotiatedVersions) |
There was a problem hiding this comment.
Hmm, yeah. Not beautiful, but its all part of this legacy module construction so I'm fine with it
| // reverseCallbacks returns a copy of the callbacks in reverse order. | ||
| // the majority of handlers are called in reverse order, so this can be used | ||
| // in those cases to prevent needing to iterate backwards over the callbacks. | ||
| func (im *LegacyIBCModule) reverseCallbacks() []ClassicIBCModule { |
There was a problem hiding this comment.
sgtm, I can't think of any better naming tbh
chatton
deleted the
cian/issue#7135-add-convenience-func-to-reverse-callbacks-and-refactor-other-methods
branch
Description
closes: #7135
This PR adds a reversal function and refactors the callbacks to reduce the amount of index manipulation used through the implementations.
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.