[boost] Re: Formal Review: Indexed Set

From: Fredrik Blomqvist (fredrik_blomqvist_at_[hidden])
Date: 2004-03-30 16:40:31

Hi

This is my review of the indexed set library.

* What is your evaluation of the design?

Overall I find the design well thought out. I'm impressed by the amount of
functionality available without "bloat".

I don't feel quite comfortable with the assymetry of index-0 not needing a
get<> accessor. I'd prefer forcing the use of get<> everywhere but I guess I
can live with it if thats decided. More real-world usage should reveal
wether a substantional amount of use-cases naturally benefit from this bias.

I also second the requests for lifting an implementation of a
bidirectional_map/set to official status. But I don't expect nor demand this
to be added before acceptance, I'm fine with it being a reasonable
prioritized roadmap feature.

Serialization support is also high on my wish-list (but with same remarks as
above).

* What is your evaluation of the implementation?

I only browsed the source and it looked good (but see the coding style issue
below)

-I think it would be better if the iterators (index_iterator.hpp) were
generated using boost.iterator_facade/adapter instead of the boost.operators
helper. This would make it easier to adapt and for example directly support
the new iterator concepts.

-This is more of a "feeling", but could perhaps (parts of?) the 'member'
functionality be refactored to use boost::bind/mem_fn code instead?

* What is your evaluation of the documentation?

I found the documentation to be very good. Stylish, with nice examples and
"bonus-material" like a performance section and an encouraging roadmap!

Some nitpicks:
-In the example at the end of the tutorials section ('Projection of
iterators')
I think it would be clearer if it would use equal_range instead.

-I find the coding style to be slightly too compact. No space after ',' and
'&' etc makes it more difficult to quickly browse. Why not just follow the
semi-official boost coding guidelines? (currently found in the file area)

-Several of the code example in the tutorial section could be slightly
reformatted to use less horizontal screen space (see for example the 'Key
extraction' part).
I'd rather scroll vertically than horizontally! Simply pulling in some of
the end of line comments would fix most of it.

* What is your evaluation of the potential usefulness of the library?

Very useful! Many times I've had the need for a container like this. (I even
found an old sketchbook of mine illustrating this storage<->views concept,
(it was marked Todo.. ;-)Synchronized views, bidirectionality etc can now
quickly and _robustly_ be added to programs, greatly simplifiying code And
improving efficiency.

* Did you try to use the library?  With what compiler?  Did you have any
problems?

Yes, altough not as much as I hoped :(  I did some simpler tests based on
examples and then I added bidirectional lookup to a smaller program
previously using a std::set<> + brute-force.. ;-)
I used VC7.1 and experienced no particular problems.
Hope to be able to use it in larger scale shortly.

* How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

Something inbetween, diffucult estimating nr of hours. I did download and
browsed docs etc of many of the intermediate versions appearing during the
development also.

* Are you knowledgeable about the problem domain?

I'm fairly fluent in STL and most of Boost. I've had ideas for this kind of
storage previously but never got around to research and implement anything
this comprehensive.

* Do you think the library should be accepted as a Boost library?

Yes, I vote for acceptance.

I'm sure I'll be using this fine lib in many apps to come.
I thank Joaquín for his efforts!

Regards
//Fredrik Blomqvist

(I pass the naming issues since I don't feel I've anything to add that
hasn't been said already.)