Counterproposal to 182733007: Add iterator to SkTDynamicHash

Counterproposal to 182733007: Add iterator to SkTDynamicHash

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=13663

[mtklein, 2014-03-04 23:01:35]

I'm sorry, I couldn't help myself.

[egdaniel, 2014-03-04 23:56:11]

Overall I like this version, minus my done() qualms above.

On 2014/03/04 23:01:35, mtklein wrote:
> I'm sorry, I couldn't help myself.

[egdaniel, 2014-03-04 23:56:29]

https://codereview.chromium.org/177263005/diff/2/tests/DynamicHashTest.cpp
File tests/DynamicHashTest.cpp (right):

https://codereview.chromium.org/177263005/diff/2/tests/DynamicHashTest.cpp#ne...
tests/DynamicHashTest.cpp:144: for (Hash::Iter iter(&hash); !iter.done();
++iter) {
Part of me feels like this done() function name is not commonly used in
iterators and thus may be less obvious when trying to use. But then again it
really isn't much different from a begin/end construct, just different names. So
I can be swayed either way.

[mtklein, 2014-03-05 00:13:05]

https://codereview.chromium.org/177263005/diff/2/tests/DynamicHashTest.cpp
File tests/DynamicHashTest.cpp (right):

https://codereview.chromium.org/177263005/diff/2/tests/DynamicHashTest.cpp#ne...
tests/DynamicHashTest.cpp:144: for (Hash::Iter iter(&hash); !iter.done();
++iter) {
On 2014/03/04 23:56:34, egdaniel wrote:
> Part of me feels like this done() function name is not commonly used in
> iterators and thus may be less obvious when trying to use. But then again it
> really isn't much different from a begin/end construct, just different names.
So
> I can be swayed either way.

Yeah, I feel you.  I think .done() is actually the common idiom in Skia.  Try a
git grep done\(\) to have a look.  I'd have stuck with operator bool(), but I
realized that made the code inside Iter itself really hard to follow.

[egdaniel, 2014-03-05 00:33:43]

lgtm after being convinced about done()

[mtklein, 2014-03-05 00:46:14]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-03-05 00:46:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-05 00:46:25]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mtklein, 2014-03-05 00:46:44]

lgtm

[mtklein, 2014-03-05 00:46:51]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-03-05 00:46:56]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/177263005/2

[commit-bot: I haz the power, 2014-03-05 01:00:55]

Change committed as 13663