Re: [boost] Re: Formal Review: Indexed Set

From: JOAQUIN LOPEZ MU?Z (joaquin_at_[hidden])
Date: 2004-03-30 17:42:34

Hi Fredrik, thank you for the reviewing effort!

----- Mensaje original -----
De: Fredrik Blomqvist <fredrik_blomqvist_at_[hidden]>
Fecha: Martes, Marzo 30, 2004 11:40 pm
Asunto: [boost] Re: Formal Review: Indexed Set

[...]
> * What is your evaluation of the design?
> 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.

Others have also expressed dislike for this feature.
Admittedly, it is easier to go the other way around:
releasing first without default get<0> and gaining experience
to see if it's worth adding on a later release.
OTOH, I find the feature useful... I don't know.

> 
> 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.

Count on it. I'm emotionally attached to bimap, as this
container was that started it all.

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

It is already in the todo list. I'll try to reserve some
time to enter into the serialization review and use this
case as a testbed.

> * What is your evaluation of the implementation?
[...]
> -I think it would be better if the iterators (index_iterator.hpp) were
> generated using boost.iterator_facade/adapter instead of the 
> boost.operatorshelper. This would make it easier to adapt and for 
> example directly support
> the new iterator concepts.

Ummm... will look at it.

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

I don't think so. member<> and the other key extractors
are polymorhic, meaning that they accept not only
objects of type T, but also reference_wrapper<T>'s, pointers
to T and some more. I cannot figure out how to do this
with boost::bind.

> * What is your evaluation of the documentation?
[...]
> 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.

You cannot! I was bitten by this at my first try
to write the snippet. Suppose we do an equal_range()
on "sister" and get the range [it0,it1): it0 will
point to a sister and *it1 will point one-past the
last occurrence of "sister". Imagine now that 
we are prepending "younger" instead of "older"
and that *it1 is "zoe". Then

while(it0!=it1;++it0)
{
  // get it2 from it0 by means of project()
  tc.insert(it2,"younger");
}

kah-boom! Now, *as seen by index #1*, "younger"
gets between the last occurrence of "sister"
and "zoe" (alphabetical order). So it1
will be pointing off beyond the "sister"
range (in fact, an infinite recursion ensues.)
I could have fixed this by considering
(--it1) instead, but I thought the way I settled
on was clearer for explanatory purposes.

> 
> -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)

No particular reason for the style, I've been coding
like this for 10 yrs. Other reviewer requested that
lines did not exceed 80 chars, which I've just done.
But inserting the missing blanks is a pain in the ass,
and I'll probably start coding again like before as
soon as I forget about it.

> 
> -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.
> 

Previously reported request, it's in my todo list.

> * What is your evaluation of the potential usefulness of the library?
[...] 
> * Did you try to use the library?  With what compiler?  Did you 
[...]
> Hope to be able to use it in larger scale shortly.

Please do it and report your experience.

> * How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
[...]
> * Are you knowledgeable about the problem domain?
[..]
> * Do you think the library should be accepted as a Boost library?
> 
> Yes, I vote for acceptance.

Well, thank you! I hope you can find this lib of
some use in the future.

Regards,

Joaquín M López Muñoz
Telefónica, Investigación y Desarrollo