Split TemplateHashMapImpl::Lookup into two methods

Split TemplateHashMapImpl::Lookup into two methods

This avoids both a mysterious boolean argument ("insert") and lets
non-mutating lookups skip passing an allocator (in one such case,
we were passing a scary-looking ZoneAllocationPolicy(NULL)!).

Committed: https://crrev.com/5277c41044d94ce6452e5fcf6ed703d6c1dcfe4b
Cr-Commit-Position: refs/heads/master@{#27799}

[adamk, 2015-04-09 19:38:53]

adamk@chromium.org changed reviewers:
+ marja@chromium.org, yangguo@google.com

[adamk, 2015-04-09 19:38:54]

Reviewers picked mostly at random, as this file hasn't changed much recently.

[marja, 2015-04-10 07:59:58]

lgtm, I think this API is clearer than the boolean parameter.

https://codereview.chromium.org/1074943002/diff/20001/src/hashmap.h
File src/hashmap.h (right):

https://codereview.chromium.org/1074943002/diff/20001/src/hashmap.h#newcode118
src/hashmap.h:118: return p->key != NULL ? p : NULL;
We seem to be migrating to use nullptr instead of NULL in new code, so you could
use that here.

https://codereview.chromium.org/1074943002/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (left):

https://codereview.chromium.org/1074943002/diff/20001/test/cctest/test-api.cc...
test/cctest/test-api.cc:12387: CHECK(entry != NULL);
Why did you remove this CHECK?

[yangguo, 2015-04-10 20:47:52]

On 2015/04/10 07:59:58, marja wrote:
> lgtm, I think this API is clearer than the boolean parameter.
> 
> https://codereview.chromium.org/1074943002/diff/20001/src/hashmap.h
> File src/hashmap.h (right):
> 
> https://codereview.chromium.org/1074943002/diff/20001/src/hashmap.h#newcode118
> src/hashmap.h:118: return p->key != NULL ? p : NULL;
> We seem to be migrating to use nullptr instead of NULL in new code, so you
could
> use that here.
> 
> https://codereview.chromium.org/1074943002/diff/20001/test/cctest/test-api.cc
> File test/cctest/test-api.cc (left):
> 
>
https://codereview.chromium.org/1074943002/diff/20001/test/cctest/test-api.cc...
> test/cctest/test-api.cc:12387: CHECK(entry != NULL);
> Why did you remove this CHECK?

lgtm.

[yangguo, 2015-04-10 20:47:53]

On 2015/04/10 07:59:58, marja wrote:
> lgtm, I think this API is clearer than the boolean parameter.
> 
> https://codereview.chromium.org/1074943002/diff/20001/src/hashmap.h
> File src/hashmap.h (right):
> 
> https://codereview.chromium.org/1074943002/diff/20001/src/hashmap.h#newcode118
> src/hashmap.h:118: return p->key != NULL ? p : NULL;
> We seem to be migrating to use nullptr instead of NULL in new code, so you
could
> use that here.
> 
> https://codereview.chromium.org/1074943002/diff/20001/test/cctest/test-api.cc
> File test/cctest/test-api.cc (left):
> 
>
https://codereview.chromium.org/1074943002/diff/20001/test/cctest/test-api.cc...
> test/cctest/test-api.cc:12387: CHECK(entry != NULL);
> Why did you remove this CHECK?

lgtm.

[adamk, 2015-04-13 18:34:41]

The CQ bit was checked by adamk@chromium.org

[adamk, 2015-04-13 18:34:42]

https://codereview.chromium.org/1074943002/diff/20001/src/hashmap.h
File src/hashmap.h (right):

https://codereview.chromium.org/1074943002/diff/20001/src/hashmap.h#newcode118
src/hashmap.h:118: return p->key != NULL ? p : NULL;
On 2015/04/10 at 07:59:58, marja wrote:
> We seem to be migrating to use nullptr instead of NULL in new code, so you
could use that here.

I'd favor consistency here, if that's ok with you; NULL is used throughout the
file.

https://codereview.chromium.org/1074943002/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (left):

https://codereview.chromium.org/1074943002/diff/20001/test/cctest/test-api.cc...
test/cctest/test-api.cc:12387: CHECK(entry != NULL);
On 2015/04/10 at 07:59:58, marja wrote:
> Why did you remove this CHECK?

Basically because it's testing internal invariants of the HashMap
implementation, which doesn't seem appropriate for this test (we have HashMap
tests for that). I've left similar DCHECKs around in production code, though,
which maybe I should remove too. But this one struck me as particularly wrong
(and harmless to remove).

[commit-bot: I haz the power, 2015-04-13 18:35:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1074943002/20001

[commit-bot: I haz the power, 2015-04-13 19:01:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-13 19:01:32]

Patchset 2 (id:??) landed as
https://crrev.com/5277c41044d94ce6452e5fcf6ed703d6c1dcfe4b
Cr-Commit-Position: refs/heads/master@{#27799}