Quick code review: provide a real std::string hash, not just a forward declaration for something that doesn't exist
Mark Mentovai
change Mac Chromium to build at -O2, and for some reason, this code
builds at -Os and -O0 but not -O2. You've provided a declaration for
a hash function which libstdc++ provides as an inline function in a
header, it's not actually present in libstdc++.dylib. For reasons
explained in the comment in the patch, we can't just #include the
header that provides the hash function, so I've replaced the
declaration with the actual hash implementation.
This is exactly what we do in Chromium. If you're interested, see
Chromium's src/base/hash_tables.h.
I'll probably TBR this.
Mark
Neal Sidhwaney
Sounds good to me, but can you use rietveld?
http://codereview.appspot.com/static/upload.py
Use the following settings:
upload.py --server=breakpad.appspot.com
THanks,
Neal
Jim Blandy
Actually, hash_map is deprecated, and the newer GNU C++ compilers
complain about #including that header file; I ran into this when I
began to use the DWARF reader on Linux. There is a new unordered_map
container in TR1, but it simplifies the code the most just to use an
ordinary map instead of hash_map. Since this is only used for looking
up object file sections, I doubt the difference matters.
I've filed an alternative patch --- mostly deleting code:
http://breakpad.appspot.com/23001
Mark Mentovai
> Actually, hash_map is deprecated, and the newer GNU C++ compilers
> complain about #including that header file; I ran into this when I
> began to use the DWARF reader on Linux. There is a new unordered_map
> container in TR1, but it simplifies the code the most just to use an
> ordinary map instead of hash_map. Since this is only used for looking
> up object file sections, I doubt the difference matters.
>
> I've filed an alternative patch --- mostly deleting code:
> http://breakpad.appspot.com/23001
Right. We've got additional constraints in Chrome that require us to
stick with hash_map for the time being. If map is sufficient for your
use in Breakpad (and it ought to be), then your patch LGTM.
Mark
Jim Blandy
Huh --- it would be nice to eventually share DWARF code between the
two projects. Why does Chrome need to use hash_map?
Mark Mentovai
> Huh --- it would be nice to eventually share DWARF code between the
> two projects. Why does Chrome need to use hash_map?
hash_map as opposed to map: performance, in applications where it
matters. map itself is fine when you don't need the hash, as is the
case in this instance in Breakpad.
hash_map as opposed to unordered_map: not all of our compilers (or
their libraries) support the latter. We can use hash_map pretty
easily based on MSVC++'s <hash_map> stdext::hash_map, and gcc's
<ext/hash_map> __gnu_cxx::hash_map. As long as we provide hash
functions for the gcc version as needed, the two interfaces are
compatible.
The long-term goal is to use <tr1/unordered_map> across the board.
Mark
Jim Blandy
>
> Jim Blandy wrote:
>> Huh --- it would be nice to eventually share DWARF code between the
>> two projects. Why does Chrome need to use hash_map?
>
> hash_map as opposed to map: performance, in applications where it
> matters. map itself is fine when you don't need the hash, as is the
> case in this instance in Breakpad.
Oh, I can readily believe that Chrome needs hash tables and not maps
elsewhere. As long as we can converge on this particular case in the
DWARF reader, I'm happy for now.
Mark Mentovai
> Oh, I can readily believe that Chrome needs hash tables and not maps
> elsewhere. As long as we can converge on this particular case in the
> DWARF reader, I'm happy for now.
std::map will be fine here. We don't impose too many requirements on
"external" code like Breakpad. We supply our own build file to
integrate it into our system, but other than that, we're using it
completely unmodified.
Mark