Type of Review for Unordered Associative Containers & Hash Functions
Hello everyone, I've just uploaded a new version of the unordered associative containers, this time in the 'Hash' folder on the file vault. There's also the file 'hash.tar.gz', which is just the hash function object. The hash function objects are ready for review, the container library is almost ready, it mainly just needs more documentation. The implementation has been split into two libraries. Boost.Hash implements the hash function objects which would go into <functional>, so it is proposed that these are added to the functional library. This is needed for Joaquín's MultiIndex library, so it would be good to add it to Boost as soon as possible. The other part is Boost.Unordered, which implements the unordered associative containers. This is a little larger and more complicated, and there's no immediate need for it (unless you count Boost.Tr1). We're not sure what kind of review these libraries will need. Since they're both based on the draft standard, their design isn't really up for review here (right?), but the documentation and implementation is. Do you think they require a full review, or will a fast track review be appropriate? A fast track review is tentatively scheduled for the hash function objects after the review of the finite state machine, with Thorsten managing it. The full container library could be included in this, if that is considered acceptable. Or it could be added to the queue for a full review? Any opinions? Daniel
On Feb 23, 2005, at 12:36 PM, Daniel James wrote:
The implementation has been split into two libraries. Boost.Hash implements the hash function objects which would go into <functional>, so it is proposed that these are added to the functional library. This is needed for Joaquín's MultiIndex library, so it would be good to add it to Boost as soon as possible.
If it's small and MultiIndex needs it, it can go right in.
We're not sure what kind of review these libraries will need. Since they're both based on the draft standard, their design isn't really up for review here (right?), but the documentation and implementation is. Do you think they require a full review, or will a fast track review be appropriate?
Boost.Unordered should be a fast-track review. We're only looking at code, tests, and docs: the interface is fixed or we're talking to the wrong crowd. Doug
Douglas Gregor wrote:
Boost.Unordered should be a fast-track review. We're only looking at code, tests, and docs: the interface is fixed or we're talking to the wrong crowd.
This is a little pedantic, but the interface isn't entirely fixed. There are a couple of cases where it is affected by active issues. First, there's the behaviour of swap when allocators aren't equal, as described in: http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2004/n1599.html Secondly, there's the return type of erase(const_iterator) and erase(const_iterator, const_iterator). In the draft they return void but Thorsten asked if I could change them to return an iterator, as described in: http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2005/n1753.html#130 This has a small performance hit with small nodes, as erase has to search for the next empty bucket to return the iterator. But seems like a good idea to me. These are small points, but should be included in the review. (I'll add something to the documentation). Daniel
Daniel James ha escrito:
Hello everyone,
I've just uploaded a new version of the unordered associative containers, this time in the 'Hash' folder on the file vault. There's also the file 'hash.tar.gz', which is just the hash function object. The hash function objects are ready for review, the container library is almost ready, it mainly just needs more documentation.
The implementation has been split into two libraries. Boost.Hash implements the hash function objects which would go into <functional>, so it is proposed that these are added to the functional library. This is needed for Joaquín's MultiIndex library, so it would be good to add it to Boost as soon as possible.
The other part is Boost.Unordered, which implements the unordered associative containers. This is a little larger and more complicated, and there's no immediate need for it (unless you count Boost.Tr1).
We're not sure what kind of review these libraries will need. Since they're both based on the draft standard, their design isn't really up for review here (right?), but the documentation and implementation is. Do you think they require a full review, or will a fast track review be appropriate?
A fast track review is tentatively scheduled for the hash function objects after the review of the finite state machine, with Thorsten managing it. The full container library could be included in this, if that is considered acceptable. Or it could be added to the queue for a full review?
Any opinions?
My opinion is that Boost.Hash should be fasttrack reviewed (Doug went for immediate committing, but IMHO a little review cannot hurt.) As for BoostUnordered, I really don't think it qualifies for fasttrack review (see the requirements at http://boost.org/more/formal_review_process.htm#Fast-Track). Nonetheless, it'd probably be possible to shedule a shorter than normal review, since as Daniel points out, the design is pretty much out of question. My 2c, Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
"Joaquín Mª López Muñoz" <joaquin@tid.es> wrote in message news:421D9494.A06BA029@tid.es... |As for BoostUnordered, I really don't think it qualifies for fasttrack |review (see the requirements at |http://boost.org/more/formal_review_process.htm#Fast-Track). yeah, that definition does not take into account things in the tr that are not in boost. I can't see any good reason this should not be a fast-track review. -Thorsten
I can't see any good reason this should not be a fast-track review.
Well, although I'm new to review processes (so forgive me any great mistake in this mail), I think there are some very interesting issues. After reviewing a bit I've found two interesting themes (apart from some VC 7.1 warning issues): allocator swapping (what if allocators are not equal but can be or not swappable) and the usage of 2 or 3 allocators to avoid temporaries. I don't know if this is enough to discard a fast track, but in boost you never know how interesting a theme can be. Anyway, code seems much more polished and there are some very helpful helping-classes like allocator_construct that could be very useful when building new containers (for example, future associative vector and trees). Ion P.D.: I've just discovered the rebind_to way to make VC6 compatible with rebind allocators. Good god, some people really have talent!
"Thorsten Ottosen" <nesotto@cs.auc.dk> writes:
"Joaquín Mª López Muñoz" <joaquin@tid.es> wrote in message news:421D9494.A06BA029@tid.es...
|As for BoostUnordered, I really don't think it qualifies for fasttrack |review (see the requirements at |http://boost.org/more/formal_review_process.htm#Fast-Track).
yeah, that definition does not take into account things in the tr that are not in boost.
I can't see any good reason this should not be a fast-track review.
Fast track reviews were supposed to be reserved for components that are already in use as an implementation detail by one or more boost libraries, and that we now want to make publicly-accessible. If we are going to change that policy, we ought to be very clear about _why_ we're doing that, and just what the new policy should be. -- Dave Abrahams Boost Consulting www.boost-consulting.com
"David Abrahams" <dave@boost-consulting.com> wrote in message news:uu0o0641j.fsf@boost-consulting.com... "Thorsten Ottosen" <nesotto@cs.auc.dk> writes:
"Joaquín Mª López Muñoz" <joaquin@tid.es> wrote in message news:421D9494.A06BA029@tid.es...
|As for BoostUnordered, I really don't think it qualifies for fasttrack |review (see the requirements at |http://boost.org/more/formal_review_process.htm#Fast-Track).
yeah, that definition does not take into account things in the tr that are not in boost.
I can't see any good reason this should not be a fast-track review.
| Fast track reviews were supposed to be reserved for components that | are already in use as an implementation detail by one or more boost | libraries, and that we now want to make publicly-accessible. If we | are going to change that policy, we ought to be very clear about _why_ | we're doing that, and just what the new policy should be. I think tr components should be allowed special status. What's the point in putting tr components through a normal review, when there can be no interface changes? If the definition of fast-track review is not suitable for tr components, then let us call it a tr review. The important aspect of such a review is that it should be fast. -Thorsten
Thorsten Ottosen wrote:
I think tr components should be allowed special status. What's the point in putting tr components through a normal review, when there can be no interface changes?
If the definition of fast-track review is not suitable for tr components, then let us call it a tr review. The important aspect of such a review is that it should be fast.
Fast as in soon? Or just spending less time on the review? I don't mind waiting in the review queue so unless anyone really wants to see this in Boost as soon as possible, I guess that Boost.Hash should be fast tracked, Boost.Unordered should go in the queue, maybe for a shorter than normal review. Is that okay with everyone? Daniel
"Daniel James" <daniel@calamity.org.uk> wrote in message news:cvq8hm$cbj$2@sea.gmane.org... | Thorsten Ottosen wrote: | > I think tr components should be allowed special status. What's the point in | > putting tr components through a normal review, when there can be no interface | > changes? | > | > If the definition of fast-track review is not suitable for tr components, then | > let us call it a | > tr review. The important aspect of such a review is that it should be fast. | | Fast as in soon? Or just spending less time on the review? the latter. | I don't mind | waiting in the review queue so unless anyone really wants to see this in | Boost as soon as possible, I guess that Boost.Hash should be fast | tracked, Boost.Unordered should go in the queue, maybe for a shorter | than normal review. Is that okay with everyone? again, what is the difference---in time-- between a fast-track review and a review that is "a shorter than normal review" ??? -Thorsten
On Sun, 27 Feb 2005 01:41:19 +0100, Thorsten Ottosen wrote
again, what is the difference---in time-- between a fast-track review and a review that is "a shorter than normal review" ???
The review wizard has discretion on the exact length, but I think we are currently using 10 days for normal reviews. I would expect a fast-track review might be half that time -- but again, nothing is fixed in stone. Also, we may want to bump this ahead in the queue -- but again that's really a decision for the review wizard to coordinate. Jeff
participants (7)
-
Daniel James -
David Abrahams -
Douglas Gregor -
Ion Gaztañaga -
Jeff Garland -
Joaquín Mª López Muñoz -
Thorsten Ottosen