Re: [boost] [review] Review of Unordered library started December 7 and still running!

From: Steven Watanabe (steven_at_[hidden])
Date: 2007-12-20 11:13:21

AMDG

Ion Gaztañaga <igaztanaga <at> gmail.com> writes:

> 
> Hi all,
> 
> The formal review of the Unordered library started December 7, we have a 
> few nice reviews, but they are not enough. If you are interested in the 
> library (and I *know* you are), please take some time to review it.

Sorry I didn't do a full review earlier.  I just have a few
implementation comments.

allocator.hpp
line 118:
            typedef typename Allocator::value_type value_type;
Is there a reason you're not using allocator_value_type?

lines 165 and 217:
                reset(ptr_);
I don't think you want ADL here.

hash_table.hpp:
line 66: float_to_size_t:

I don't think the test used is correct.  The following program prints "0"
under msvc 8.0:

#include <limits>
#include <iostream>

int main() {
    std::cout << static_cast<std::size_t>(
    static_cast<float>(std::numeric_limits<std::size_t>::max())) << std::endl;
}

hash_table_impl.hpp

lines 137-140:
Do you want ADL for hash_swap?

line 865:
Is there a reason not to use cached_begin_bucket_?

line 1282:
                return float_to_size_t(ceil(
should qualify ceil with std::

line 1361:
       rehash_impl(static_cast<size_type>(floor(n / mlf_ * 1.25)) + 1);
*std::*floor?

The implementation files use BOOST_DEDUCED_TYPENAME but
unordered_map and unordered_set use typename.  Could you
make it consistant?

I'm getting a lot of warnings on the tests from msvc 8.0 with /W4
because minimal::ptr/const_ptr only defines operator+(int) and the internals
call operator+ with a size_t.  Is + required to work for the size_type
or should it be cast to the difference_type explicitly?  Also, minimal::ptr
should use std::ptrdiff_t rather than int.

In Christ,
Steven Watanabe