[review] Review of Unordered library started December 7 and still running!
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. This is the info you need to do a review: *Description*: An implementation of the unordered containers specified in TR1, with most of the changes from the recent draft standards. *Author*: Daniel James *Download*: http://www.boost-consulting.com/vault/index.php?action=downloadfile&filename=unordered.zip&directory=Containers The documentation is in the zip file. I've put documentation online here: http://igaztanaga.drivehq.com/unordered http://tinyurl.com/yw8wjp What to include in Review Comments ================================== Your comments may be brief or lengthy, but basically the Review Manager needs your evaluation of the library. If you identify problems along the way, please note if they are minor, serious, or showstoppers. Here are some questions you might want to answer in your review: * What is your evaluation of the design? * What is your evaluation of the implementation? * What is your evaluation of the documentation? * What is your evaluation of the potential usefulness of the library? * Did you try to use the library? With what compiler? Did you have any problems? * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? * Are you knowledgeable about the problem domain? And finally, every review should answer this question: * Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. Ion Gaztañaga - Review Manager -
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Ion Gaztañaga Sent: 11 December 2007 20:38 To: boost@lists.boost.org; Boost User List Subject: [boost] [review] Review of Unordered library started December 7 and still running!
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),
An implementation of the unordered containers specified in TR1, with most of the changes from the recent draft standards.
* What is your evaluation of the design? Fine. Wider use of a cross platform implementation will help WG21 refine the details of The Standard. * What is your evaluation of the implementation? OK - but not well qualified to judge. * What is your evaluation of the documentation? OK - more examples are always nice but since this is well known not vital. * What is your evaluation of the potential usefulness of the library? Invaluable, sometimes. * Did you try to use the library? With what compiler? No. Did you have any problems? No * How much effort did you put into your evaluation? Glance A glance. * Are you knowledgeable about the problem domain? a nanoscopic amount ;-)
And finally, every review should answer this question:
* Do you think the library should be accepted as a Boost library?
FWIW - Yes. Paul --- Paul A Bristow Prizet Farmhouse, Kendal, Cumbria UK LA8 8AB +44 1539561830 & SMS, Mobile +44 7714 330204 & SMS pbristow@hetp.u-net.com
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
Hi Steve, The answer to most of your questions is 'no, no reason', 'no, I don't want ADL' or 'okay, I'll do that'. Apart from those below. On 20/12/2007, Steven Watanabe <steven@providere-consulting.com> wrote:
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; }
I'll look into this soon (I don't have access to windows at home right now).
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?
Both of these are preceded by 'use namespace std'. I was trying to support libraries that don't define the functions in the std namespace. ADL shouldn't be a problem here as the arguments are floats. But I could use the compatibility headers instead.
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?
I don't think the standard specifies exactly what the minimum requirements for 'Allocator::pointer' are. But I'd expect operator+ to use difference_type so I'll add casts. Maybe I should check for allocators which return a 'max_size' which doesn't fit into difference_type. Daniel
AMDG Daniel James <daniel_james <at> fmail.co.uk> writes:
I'll look into this soon (I don't have access to windows at home right now).
I think that it should work correctly if you replace line 68 f > static_cast<float>((std::numeric_limits<std::size_t>::max)() with f >= static_cast<float>((std::numeric_limits<std::size_t>::max)()
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?
Both of these are preceded by 'use namespace std'.
I don't see a using namespace std on those two lines... In Christ, Steven Watanabe
participants (4)
-
Daniel James -
Ion Gaztañaga -
Paul A Bristow -
Steven Watanabe