From: Jamie Allsop (ja11sop_at_[hidden])
Date: 2007-12-18 22:04:07
First apologies for the belated review and its brevity - I really had
hoped I'd have time to be more thorough, but here goes.
First some comments on the docs (nits/suggestions really). The
documentation is really very good.
--- Introduction 2nd para - "For a hash table you need an equality function and a hash function for the key." I got the impression you were contrasting this with requirements of the ordered containers requiring less than comparable values. If that is so then you might want to use something like, "In contrast a hash table ..." of "For a hash table you _only_ need ..." as you are using this to point out an advantage of hash tables. 3rd para - "So the C++ Standard Library Technical Report introduced ..." I think needs re-worded slightly. 'So' presumably refers to the advantages of the hashed containers. In this case it is abrupt and not very specific. Perhaps you could relate the start of the sentence to the advantages you oultine more directly. For example, "With this in mind ...", or simply start, "The C++ Library Technical Report has therefore introduced ..." Last para - "There are other differences, which will be detailed later." If there are one or two specific places where these are enumerated it would be nice to directly reference the sections. --- Data Structure 2nd para - A reference to the "Equality Predicates and Hash Functions" would be nice, if only to indicate to the reader that there is more detailed information. You could perhaps hyperlink "hash function" to that section. 3rd para - "If at a later date the container wants to find an element in the container it just has to apply the same process to the element's key to discover which bucket it is in." This rather long sentence could do with one or more commas to make reading it easier for example, "If at a later date the container wants to find an element in the container, it just has to apply the same process to the element's key to discover which bucket it is in." Better yet, try to split it into two sentences (if that is possible). 4th para - /comparison/comparisons/ --- Controlling the number of buckets 1st para - "...performance to get worse." Should this be "...performance to worsen/degrade." 2nd para - "First you can specify the minimum number of buckets in the constructor, and later, by calling rehash." Ok, so you mean that you can specify a minimum number of buckets when you construct your container, after construction you can also influence the bucket count by calling rehash? The "... ,and later, ..." threw me the first time I read it. 3rd para - "The other method is the max_load_factor member function." Ok, now I am confused... are there three methods? Or are the two methods 1 - specify the minimum number of buckets in the constructor 2 - the max_load_factor member function ...and calling rehash is relevant to both. I think this and the previous paragraph need to be made more coherent. 6th para - "Note: rehash's argument is the number of buckets..." Ok, now I think I am seeing what you want to say in the 2nd paragraph -- the number of buckets can be specified either during construction or when rehash is called? "The + 1 is required because the container is allowed to resize when the load factor is equal to the maximum load factor." For example? It's late here and I'm not sure what you mean (but I guess it will be obvious in the morning :) ) --- Equality Predicates and Hash Functions This section has been well discussed in previous posts. However on reading it I thought that an earlier explicit reference to Boost.Hash might be appropriate, spelling out the relationship between the two libraries. --- One thing that would be nice are some performance details, perhaps comparisons against some other library implementations using the same has function. Just a thought. --- The rest of the docs go into a variety of details about specific topics and is all very good. Ion Gaztañaga wrote: > * What is your evaluation of the design? Well thought out and closely tracking TR1. > * What is your evaluation of the implementation? I haven't really looked at it - if I get a chance I will do so. > * What is your evaluation of the documentation? Excellent - see above. > * What is your evaluation of the potential usefulness of the library? We will be looking to use this library in critical areas of our codebase where performance is critical > * Did you try to use the library? With what compiler? Not yet. > Did you have any problems? N/A > * How much effort did you put into your evaluation? > A glance? A quick reading? In-depth study? I read the docs a few times and looked at other references, plus reviewed the previous reviewers comments. > * Are you knowledgeable about the problem domain? So so. I haven't done a lot of work with hash containers before (I haven't needed to). I DO need to make heavy use of them now so this library is very timely. I've unfortunately not been able to get time to replace our existing implementation with this one and do a performance comparison. > > 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. This is a much needed library in boost. I vote for it to be accepted. > > Unordered containers are likely to become one of the most used Boost > libraries so I encourage you to review the library in order to see if > Unordered fulfills all your expectations. I think this appears to be an excellent start. Performance will be an issue for me, but as I am in a position to do the needed comparisons myself (but don't have time yet) I can appreciate how more performance information could be very subjective to different peoples' needs. Jamie > > Ion Gaztañaga > - Review Manager - > _______________________________________________ > Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost >
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk