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

From: Fredrik Blomqvist (fredrik_blomqvist_at_[hidden])
Date: 2004-04-01 16:44:12

JOAQUIN LOPEZ MU?Z wrote:
> Hi Fredrik, thank you for the reviewing effort!
>
> ----- Mensaje original -----
> De: Fredrik Blomqvist <fredrik_blomqvist--at--home.se>
> Fecha: Martes, Marzo 30, 2004 11:40 pm
> Asunto: [boost] Re: Formal Review: Indexed Set
>
[snip]
>> 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.
>
Great! (I did read your fine Codeproject article about it also)

>>
>> 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.
Nice to hear. Indexed_set + serialization will be powerful stuff indeed!

[Snip]
>> * 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.
>
Doh! Seems like I only looked at the iteration... totally neglected the fact
that you were modifying the set! sorry...

>>
>> -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.
>
Ah, I didn't really mean that you needed to change _all_ the main code (just
a hint ;-).
My point was mostly that the code snippets in the _documentation_ should be
changed.
Still slightly tedious I guess..
(hmm, a boost::pretty-printer based on spirit tech would be a cool little
util :) )

Regards
// Fredrik Blomqvist