Pool TypeName strings when writing MINIDUMP_HANDLE_DESCRIPTOR

Pool TypeName strings when writing MINIDUMP_HANDLE_DESCRIPTOR

Follow up to TODO in https://codereview.chromium.org/1419623003/.

R=mark@chromium.org
BUG=crashpad:21, crashpad:52
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/1407b21d695fbebdb2ae2f3c326360fa91d97e7c

[scottmg, 2015-10-21 00:48:42]

Description was changed from

==========
Pool TypeName strings of MINIDUMP_HANDLE_DESCRIPTOR

BUG=crashpad:21, crashpad:52
==========

to

==========
Pool TypeName strings when writing MINIDUMP_HANDLE_DESCRIPTOR

Follow up to TODO in https://codereview.chromium.org/1419623003/.

R=mark@chromium.org
BUG=crashpad:21, crashpad:52
==========

[scottmg, 2015-10-21 00:48:42]

scottmg@chromium.org changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2015-10-21 18:27:02]

https://codereview.chromium.org/1411793005/diff/40001/minidump/minidump_handl...
File minidump/minidump_handle_writer.cc (right):

https://codereview.chromium.org/1411793005/diff/40001/minidump/minidump_handl...
minidump/minidump_handle_writer.cc:51: auto it =
strings_.find(handle_snapshot.type_name);
std::map<> has a really annoying interface for this. It kind of sucks that
find() does a lookup here and then the operator[] potentially does the identical
lookup below.

Instead of find(), you could lower_bound(). Then the conditional below becomes
|it != end() && it->first == type_name|, and in the else branch, you could
insert(it, value_type(type_name, writer)).

The other alternative is to insert(value_type(type_name, writer)) up front
without first checking to see whether it’s present. If the key was already
there, the returned pair.second will be false, and pair.first will be the
existing writer. That’s nominally cleaner than the
lower_bound()+double-test+insert() approach, but since writer’s a pointer to a
newly-allocated thing, you’re still on the hook for deleting it.

Does it matter much here? Probably not. But the alternatives are probably just
as little code as this, maybe even less.

[scottmg, 2015-10-21 18:46:10]

https://codereview.chromium.org/1411793005/diff/40001/minidump/minidump_handl...
File minidump/minidump_handle_writer.cc (right):

https://codereview.chromium.org/1411793005/diff/40001/minidump/minidump_handl...
minidump/minidump_handle_writer.cc:51: auto it =
strings_.find(handle_snapshot.type_name);
On 2015/10/21 18:27:01, Mark Mentovai wrote:
> std::map<> has a really annoying interface for this. It kind of sucks that
> find() does a lookup here and then the operator[] potentially does the
identical
> lookup below.
> 
> Instead of find(), you could lower_bound(). Then the conditional below becomes
> |it != end() && it->first == type_name|, and in the else branch, you could
> insert(it, value_type(type_name, writer)).
> 
> The other alternative is to insert(value_type(type_name, writer)) up front
> without first checking to see whether it’s present. If the key was already
> there, the returned pair.second will be false, and pair.first will be the
> existing writer. That’s nominally cleaner than the
> lower_bound()+double-test+insert() approach, but since writer’s a pointer to a
> newly-allocated thing, you’re still on the hook for deleting it.
> 
> Does it matter much here? Probably not. But the alternatives are probably just
> as little code as this, maybe even less.

Done.

[Mark Mentovai, 2015-10-21 20:14:02]

LGTM. Excellent, this was even less work than I had expected.

[scottmg, 2015-10-21 20:25:55]

Committed patchset #6 (id:100001) manually as
1407b21d695fbebdb2ae2f3c326360fa91d97e7c (presubmit successful).