Factor bucket chain search into walkBucketChain
ClosedPublic

Authored by olivierjg on Aug 28 2015, 8:25 PM.

Details

Reviewers
mwolff
Group Reviewers
KDevelop
Summary

For now just used for index/findIndex, but there are more uses for walkBucketChain

Diff Detail

Repository
R33 KDevPlatform
Lint
Lint Skipped
Unit
Unit Tests Skipped
olivierjg updated this revision to Diff 632.Aug 28 2015, 8:25 PM
olivierjg retitled this revision from to Factor bucket chain search into walkBucketChain.
olivierjg updated this object.
olivierjg edited the test plan for this revision. (Show Details)
olivierjg added a reviewer: KDevelop.
olivierjg set the repository for this revision to R33 KDevPlatform.
olivierjg added a project: KDevelop.
olivierjg added a subscriber: KDevelop.
mwolff added a subscriber: mwolff.Aug 31 2015, 9:34 AM
mwolff added inline comments.
serialization/itemrepository.h
1354–1357

replace [&] with [this, request].

and as I say below, I find it odd that the return value decides both whether the loop is stopped, and the return value of the walkBucketChain method.

1833

some documentation please. and I find the choice of return types a bit odd.

the lambda should always return true/false, no? and that would then tell the loop to break or not. the walkBucketChain could then return void. The return value of the lambda could be given out via a captured variable?

olivierjg updated this revision to Diff 925.Sep 27 2015, 7:34 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptSep 27 2015, 7:34 AM
olivierjg added inline comments.Sep 27 2015, 7:49 AM
serialization/itemrepository.h
1833

I do understand what you're saying by return types being odd, but having walkBucketChain return void or bool is not really a readability improvement in any situation.

We don't want void, since that means we /require/ a sentinel value to tell us if we broke early or found nothing, which won't necessarily be useful otherwise. Ie, in deleteItem, if found we set one value, else another one, so we'd either have to initialize it to zero and then set it again if found, or add another bool to tell us that it was found.

We could just have walkBucketChain return, but aside from just being noisier, it seems like premature pessimization. I think for this case where we are always working with pointers or zero-invalid bucket indexes, it makes some sense to have it as it is. Of course if you still find it undesirable, I'll change it.

minor nitpicks, I'm in favor of this. How do I give a "ship it" on phabricator?

serialization/itemrepository.h
1825

put brace on newline

mwolff accepted this revision.Oct 8 2015, 10:13 AM
mwolff added a reviewer: mwolff.

accepting

This revision is now accepted and ready to land.Oct 8 2015, 10:13 AM
olivierjg closed this revision.Nov 3 2015, 8:47 PM

Already submitted.