Chore: Use exp/slices and exp/maps to simplify some code #5479
Conversation
| r := make([]io.Closer, len(closers)) | ||
| copy(r, closers) | ||
| return &multiCloser{r} | ||
| return &multiCloser{slices.Clone(closers)} |
There was a problem hiding this comment.
I'm not even sure this needs the clone. When called as varargs, the slice is surely unshared.
jannotti
force-pushed
the
generic-stuff
branch
3 times, most recently
from
b874496
to
592d035
Compare
Codecov Report
@@ Coverage Diff @@
## master #5479 +/- ##
==========================================
- Coverage 55.71% 55.62% -0.10%
==========================================
Files 446 446
Lines 63292 63153 -139
==========================================
- Hits 35265 35130 -135
- Misses 25649 25653 +4
+ Partials 2378 2370 -8
... and 18 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| // | ||
| // known artefact of cloning AppLocalState even with empty update, nil map vs empty map | ||
| saved.AppLocalStates = map[basics.AppIndex]basics.AppLocalState{} |
There was a problem hiding this comment.
This appeared to be testing an artifact of the mock, not of a real ledger. The artifact is gone now, because maps.Clone maintains nil.
| } | ||
| } | ||
| return true | ||
| return maps.Equal(sd, o) |
There was a problem hiding this comment.
Comment goes into a lot of detail about the nil checking choice, but that is exactly the same choice that maps.Equal made. Unit tests already exist to confirm.
The intent is to only make changes where the result is clearer, and to catch enough cases that the next time someone does something similar, they will have seen the new, clearer way to write the thing they intend.
NOTE(rsc): This function does NOT call the runtime cmpstring function, because we do not want to provide any performance justification for using strings.Compare. Basically no one should use strings.Compare.
Need to do a little gitting around for the memory optimzation reversion.
|
Happy to approve once the merge conflict is fixed |
| // Fetch up to maxResults - len(res) + len(deletedCreatables) from the database, | ||
| // so we have enough extras in case creatables were deleted | ||
| numToFetch := maxResults - uint64(len(res)) + uint64(len(deletedCreatables)) | ||
| dbResults, dbRound, err := au.accountsq.ListCreatables(maxCreatableIdx, numToFetch, ctype) |
There was a problem hiding this comment.
forgot to remove func (qs *accountsDbQueries) ListCreatables in sql.go and listCreatablesStmt there as well
The intent is to only make changes where the result is clearer, and to catch enough cases that the next time someone does something similar, they will have seen the new, clearer way to write the thing they intend.