Use tr1/unordered_{map,set} instead of ext/hash_{map,set} in base/hash_tables.h

Use tr1/unordered_{map,set} instead of ext/hash_{map,set}.  This allows
removing the -Wno-deprecated warning exclusion.

HttpVersion had to be changed because glibc defines macros named "major" and
"minor" which was causing compilation problems with the change to
unordered_{map,set}.

BUG=2053

[Evan Martin, 2008-09-16 19:31:07]

I was going to try this out, but it conflicts with trunk.  Can you update and
re-upload?

[vega.james_gmail.com, 2008-09-16 19:55:13]

Updated to fix conflicts from the warning cleanup in r2272

[Mark Mentovai, 2008-09-16 20:27:44]

adding self to list

[Mark Mentovai, 2008-09-16 20:44:30]

http://codereview.chromium.org/3083/diff/6/208
File base/hash_tables.h (right):

http://codereview.chromium.org/3083/diff/6/208#newcode33
Line 33: #define hash_map unordered_map
No way.  Can you do |typedef std::tr1::unordered_map hash_map;|?

http://codereview.chromium.org/3083/diff/6/210
File net/http/http_version.h (right):

http://codereview.chromium.org/3083/diff/6/210#newcode22
Line 22: HttpVersion(uint16 vmajor, uint16 vminor) : value(vmajor << 16 |
vminor) { }
What's v?

http://codereview.chromium.org/3083/diff/6/210#newcode25
Line 25: uint16 majorVersion() const {
thisKindOfCasingNotReallyAllowedByOurStyleGuide

Simple accessors should be named entirely in lowercase, and should be named
after the field they access.  In this case, since the field is named "value", we
can't really get away with that, but we could do major_version() and
minor_verson().

Since you've changed the names in the constructor, though, and I assume that was
because of the naming conflict with these accessors, I think you could just
leave the accessors at major() and minor().

[Mark Mentovai, 2008-09-16 20:46:14]

http://codereview.chromium.org/3083/diff/6/208
File base/hash_tables.h (right):

http://codereview.chromium.org/3083/diff/6/208#newcode31
Line 31: using std::tr1::unordered_map;
Also, be sure to test the change you ultimately arrive at here with some
representative sample of compilers.  Things might not have been as nailed-down
in earlier implementations, and it's good to cover all the bases.

[vega.james_gmail.com, 2008-09-16 20:55:34]

http://codereview.chromium.org/3083/diff/6/208
File base/hash_tables.h (right):

http://codereview.chromium.org/3083/diff/6/208#newcode33
Line 33: #define hash_map unordered_map
On 2008/09/16 20:44:31, Mark Mentovai wrote:
> No way.  Can you do |typedef std::tr1::unordered_map hash_map;|?

You can't typedef an incomplete type.  That's the first thing I tried. 
unordered_map (and hash_map for that matter) are templated classes, so you'd
have to typedef every specialization that we use.

http://codereview.chromium.org/3083/diff/6/210
File net/http/http_version.h (right):

http://codereview.chromium.org/3083/diff/6/210#newcode25
Line 25: uint16 majorVersion() const {
On 2008/09/16 20:44:31, Mark Mentovai wrote:
> thisKindOfCasingNotReallyAllowedByOurStyleGuide
> 
> Simple accessors should be named entirely in lowercase, and should be named
> after the field they access.  In this case, since the field is named "value",
we
> can't really get away with that, but we could do major_version() and
> minor_verson().

Ok, that will work.

> Since you've changed the names in the constructor, though, and I assume that
was
> because of the naming conflict with these accessors, I think you could just
> leave the accessors at major() and minor().

Actually, the problem is the accessors since glibc defines major() and minor()
macros.  I could leave the names in the contstructors alone.

[Evan Martin, 2008-09-16 21:02:43]

I saw something on IRC about this getting fixed.  Or is there a workaround? 
E.g.
#ifdef major
#undef major   // work around crazy GCC macro
#endif

[Mark Mentovai, 2008-09-16 21:12:55]

http://codereview.chromium.org/3083/diff/6/208
File base/hash_tables.h (right):

http://codereview.chromium.org/3083/diff/6/208#newcode33
Line 33: #define hash_map unordered_map
jamessan wrote:
> You can't typedef an incomplete type.  That's the first thing I tried. 
> unordered_map (and hash_map for that matter) are templated classes, so you'd
> have to typedef every specialization that we use.

Of course, I should have caught that before I sent.  How about this?:

template<class Key,
         class T,
         class Hash = std::tr1::hash<Key>,
         class Pred = std::equal_to<Key>,
         class Alloc = std::allocator<std::pair<const Key, T> > >
class hash_map : public std::tr1::unordered_map<Key, T, Hash, Pred, Alloc> {
};

[vega.james_gmail.com, 2008-09-16 21:32:32]

On 2008/09/16 21:02:43, Evan Martin wrote:
> I saw something on IRC about this getting fixed.  Or is there a workaround? 
> E.g.
> #ifdef major
> #undef major   // work around crazy GCC macro
> #endif

As nasty as it is that glibc claimed the major(), minor(), and makedev() names,
I think simply renaming the functions we use is the clean approach.

[vega.james_gmail.com, 2008-09-16 21:50:58]

On 2008/09/16 21:12:55, Mark Mentovai wrote:
> template<class Key,
>          class T,
>          class Hash = std::tr1::hash<Key>,
>          class Pred = std::equal_to<Key>,
>          class Alloc = std::allocator<std::pair<const Key, T> > >
> class hash_map : public std::tr1::unordered_map<Key, T, Hash, Pred, Alloc> {
> };

Ah, right, that's much saner.  Updated CL uploaded.  I'll do some wider compiler
testing in the next couple days.

[Mark Mentovai, 2008-09-16 22:11:14]

LGTM if it tests out well, let us know what you find.

[Seo Sanghyeon, 2008-09-17 03:11:08]

On 2008/09/16 22:11:14, Mark Mentovai wrote:
> LGTM if it tests out well, let us know what you find.

Tested on GCC 4.1.2, 4.2.4, 4.3.2.

[M-A Ruel, 2008-09-17 14:57:03]

http://codereview.chromium.org/3083/diff/220/14
File base/hash_tables.h (right):

http://codereview.chromium.org/3083/diff/220/14#newcode37
Line 37: };
The main issue with these 2 classes is that the compiler will implement the
default constructor, but you need to define it so the non-default constructors
are available too. So you need:

IBM Doc:
explicit hash_map(size_type n = 3,
                  const hasher& hf = hasher(),
                  const key_equal& eql = key_equal(),
                  const allocator_type& a = allocator_type());
template <class InputIterator>
hash_map(InputIterator f, InputIterator l,
         size_type n = 3,
         const hasher& hf = hasher(),
         const key_equal& eql = key_equal(),
         const allocator_type& a = allocator_type());
hash_map(const hash_map&);
hash_map& operator=(const hash_map&);

or

Microsoft Doc:
explicit hash_map(
    size_type nbuckets = N0,
    const Hash& hfn = Hash(),
    const Pred& comp = Pred(),
    const Alloc& al = Alloc());
template<class InIt>
    hash_map(
    InIt first, InIt last,
    size_type nbuckets = N0,
    const Hash& hfn = Hash(),
    const Pred& comp = Pred(),
    const Alloc& al = Alloc());
hash_map(
    const hash_map& right);
+ operator = which is not documented.

depending on the compiler vendor. Your call on that.

[vega.james_gmail.com, 2008-09-18 15:58:14]

On 2008/09/17 14:57:03, M-A wrote:
> http://codereview.chromium.org/3083/diff/220/14
> File base/hash_tables.h (right):
> 
> http://codereview.chromium.org/3083/diff/220/14#newcode37
> Line 37: };
> The main issue with these 2 classes is that the compiler will implement the
> default constructor, but you need to define it so the non-default constructors
> are available too.

Added.  I used the default values from GCC since that's what we're using.

[M-A Ruel, 2008-09-18 16:13:24]

http://codereview.chromium.org/3083/diff/28/229
File base/hash_tables.h (right):

http://codereview.chromium.org/3083/diff/28/229#newcode20
Line 20: #if defined(COMPILER_MSVC)
// TODO(maruel): rip out all of this glue once VS2008 SP1 is the base supported
compiler on Windows.

http://codereview.chromium.org/3083/diff/28/229#newcode28
Line 28: #include <tr1/unordered_map>
I agree with deanm, if there was a way to:
GCC_DISABLE_WARNINGS()
#include <ext/hash_map>
#include <ext/hash_set>
GCC_RESTORE_WARNINGS()

I'd prefer that to this glue code.

http://codereview.chromium.org/3083/diff/28/229#newcode63
Line 63: return *this;
return static_cast<base_&>(*this) = static_cast<const base_&>(copy);
?

http://codereview.chromium.org/3083/diff/28/229#newcode95
Line 95: static_cast<base_&>(*this) = static_cast<const base_&>(copy);
return static_cast<base_&>(*this) = static_cast<const base_&>(copy);
?

[vega.james_gmail.com, 2008-09-18 16:41:14]

http://codereview.chromium.org/3083/diff/28/229
File base/hash_tables.h (right):

http://codereview.chromium.org/3083/diff/28/229#newcode28
Line 28: #include <tr1/unordered_map>
On 2008/09/18 16:13:24, M-A wrote:
> I agree with deanm, if there was a way to:
> GCC_DISABLE_WARNINGS()
> #include <ext/hash_map>
> #include <ext/hash_set>
> GCC_RESTORE_WARNINGS()
> 
> I'd prefer that to this glue code.

Well, the point of this changeset was to remove the -Wno-deprecated flag
currently being used in build/SConscript.main.  I have no problem with trimming
this changeset back to just the major()/minor() fix and revisiting the use of
unordered_* once VS2008 is the base compiler in Windows.

[vega.james_gmail.com, 2008-09-22 00:47:38]

On 2008/09/18 16:13:24, M-A wrote:
> http://codereview.chromium.org/3083/diff/28/229#newcode28
> Line 28: #include <tr1/unordered_map>
> I agree with deanm, if there was a way to:
> GCC_DISABLE_WARNINGS()
> #include <ext/hash_map>
> #include <ext/hash_set>
> GCC_RESTORE_WARNINGS()
> 
> I'd prefer that to this glue code.

GCC 4.2 and up has '#pragma GCC diagnostic' support[0] but it doesn't appear to
allow specifying "-Wdeprecated".  Therefore, I've done as I suggested in my last
email and have removed the changes to use unordered_map/unordered_set and the
only remaining changes are fixing the use of major()/minor() so they don't
conflict with glibc's macros.

[0] -
http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html#Diagnostic-Pragmas

[M-A Ruel, 2008-09-23 16:32:24]

Checked in (somewhat modified) as r2491. Please close the review.