[C++-sig] Re: indexing_v2 status update

[David Abrahams]

Raoul Gough <RaoulGough at yahoo.co.uk> writes:

> David Abrahams <dave at boost-consulting.com> writes:
>
>> Raoul Gough <RaoulGough at yahoo.co.uk> writes:
> [snip]
>> "Safety" is missing from the design goals.  Was that not one of the
>> goals?
>
> Well that's a tough one. For instance, using return_by_reference when
> returning a value held in a container is pretty dangerous (consider
> deletions or vector reallocations). 

There are supposed to be proxies which just store an index and do
range checks.  Is that what container_proxy is?

> On the other hand, I wouldn't like
> to rule it out as an option merely on that basis. Certainly the
> container_proxy component does backflips to try and prevent any
> dangers, so perhaps this needs some mention of safety?  

Yes.  If that's the safe one, it should be the default, and safety
was a major goal of Joel's original work.

> All it says at
> the moment is this:
>
>   Provide an emulation of Python reference semantics for /values/
>   in vector-like containers.
>
>>
>> I was under the impression that this:
>>
>>   Note: the suite hasn't yet been tested on a compiler without partial
>>   template specialization support. If you have any results with such a
>>   compiler (good or bad) please report them on the mailing list for
>>   the Python C++-SIG.
>>
>> is no longer true.  Wrong?
>
> Yes, this is out of date.
>
>>
>> I don't like the use of abbrevs. like "algo_...."
>
> You mean algo_selector? I can rename this to algorithm_selector if
> you'd prefer, I don't mind either way.

I prefer.

>> Overall, after just a quick look, I'm impressed, though I don't think
>> the documentation gives a clear picture of how to use all this stuff
>> yet.  I wouldn't be reeady to approve it until I can read some more
>> complete docs, so that I can understand the whole enchilada.
>>
>>   Internal policies detail
>>
>>   The container_suite object typically adds more than one function to
>>   the Python class, and not all of those functions can, or should, use
>>   exactly the same policies. For instance, the Python len method, if
>>   provided, should always return its result by value. The library
>>   actually uses up to three different sets of policies derived from
>>   the one provided to the with_policies function. These are:  
>>
>>   The supplied policies, unchanged 
>>
>>   The supplied precall policy only, using default_call_policies for
>>   result conversion.  
>>
>>   The supplied precall policies, and the supplied result conversion
>>   policies applied to each element of a returned list 
>>
>> You just drop any postcall component of supplied policies?  Rationale?
>
> The supplied postcall currently gets dropped completely for functions
> that return integers or void (e.g. __len__ and sort) on the assumption
> that it is only intended for use when returning container
> elements. For instance, you don't want to use
> return_internal_reference on the result of a call to container.size().

You need to document the postcall behaviors and the rationale.

>>   2] Note that Algorithms and ContainerTraits don't represent
>>   individual templates in the diagram, but groups of related
>>   templates. For instance, there are actually templates called
>>   list_algorithms and assoc_algorithms, among others.  
>>
>> Don't you mean that they represent concepts?
>
> Well that seems to be the Boost term for it. 

Not Boost: standard C++, STL, Generic Programming, CUJ, etc. all use
this term.

> I'd be happier using it if you could point me to a definition of the
> term, so I can embed a hyperlink to the definition. 

http://www.boost.org/more/generic_programming.html#concept

> I don't think everyone who uses the Python or indexing suite
> documentation will be familiar with it otherwise (I know I wasn't
> initially).
>>
>> "lessthan_comparable" should be "less_than_comparable".  
>
> OK.
>
>>
>> Why "visitor_helper", and not operator()?
>
> Static function.

OK.  The name sounds like it exposes implementation details.  It
should describe what it's for.

>> has_copyable_iter can be determined by looking at
>> iterator_traits<I>::iterator_category (or better,
>> iterator_traversal<I>::type).  The user shouldn't supply it.
>
> I guess this needs some explanation in the documentation - there is an
> iterator_traits template that deduces many properties automatically

Traits globs like std::iterator_traits are a bad idea; it's much
better to define individual metafunctions.

> (for instance, has_copyable_iter is indeed determined by iterator
> category). I'll add some documentation for the case that client code
> wants to use the template as a base class, although there is certainly
> no necessity for it to do so.

I don't understand why you would even give the user the option to
supply information that you can correctly deduce ??

>> I don't understand is_reorderable.  How is that different from
>>
>>   is_convertible<
>>       iterator_traits<C::iterator>::iterator_category
>>     , forward_iterator_tag
>>   >::value
>>   && is_non_const_lvalue_iterator<C::iterator>::value
>>   && is_assignable<C::value_type>::value
>>
>> ??
>>
>> I realize we don't have is_assignable, but shouldn't you phrase this
>> in terms of something fundamental like value_type assignability?  We
>> can ask all the other questions (see
>> boost/iterator/is_lvalue_iterator.hpp).
>
> The iterator traits template I mentioned above just uses
> is_mutable_ref<std_traits::reference> 

Not a good idea; the standard doesn't place any requirements on the
reference type of most iterators.

> (with an obvious is_mutable_ref
> implementation). This actually deduces the wrong answer for std::map
> and even std::set with some standard libraries, so I ended up
> overriding it manually in set_traits and map_traits.

That's why I'm suggesting using the formula above.

>> Once you tell the suite that "has_find" don't you also need to tell
>> it how (i.e. via member function or otherwise)?
>
> Yes indeed. The ContainerTraits and Algorithms have to work together
> on this - you can't have "has_find" true and then not provide an
> implementation of find, count and maybe index in your Algorithms. I
> suppose I should really document the exact requirements.

Uh, I suppose ;-)

> Also, I've recently added two static member functions to the
> ValueTraits concept for the less_than and equal_to comparisons. This
> is where the automatic shared_ptr handling happens. It's not
> documented yet.

That means little to me as I don't understand the system yet.

>> The use of static constants in ContainerTraits is unattractive to me,
>> because it's not particularly extensible.  What about asking the user
>> to provide an mpl::set of the property type tags:
>>
>>    typedef mpl::set<mutable,find,insert,push_back> capabilities;
>>
>> or something?
>
> Looks like a good idea. I'll give this a go.

We may need to whip mpl::set into shape.  In the meantime you could
use an mpl::vector, and mpl::find which will give slightly longer
compiles but should work.

>> You actually have the name "value_traits_" in a table.  Intended?
>>                                         ^
>
> There is a template called value_traits, so the ContainerTraits member
> type is called value_traits_ (underscore at end). A bit lame I guess.

Yeah, I think we should revisit these decisions.

>> Why would you ask the user to supply the Container's size_type and
>> iterator instead of deducing them from the container itself?
>
> The client could use one of the existing ContainerTraits templates as
> a base class and avoid the need for defining this explicitly (needs
> documenting, as noted above). On the other hand, it is also possible
> to interface the suite with a container that doesn't provide anything
> like an STL-style interface, by completely replacing the traits and
> algorithms templates.

The normal way to deal with that is to supply a metafunction:

    template <class C>
    struct container_value_type
    {
        typedef typename C::value_type type;
    };

And then have the user specialize it for other types.  A slightly
better implementation would look like:

      
    template <class has = mpl::false_, class C>
    struct container_value_type_impl
    {
    };

    template <class C>
    struct container_value_type_impl<mpl::true_, C>
    {
        typedef typename C::value_type type;
    };

    template <class C>
    struct container_value_type
      : container_value_type_impl<has_value_type<C>::type, C>
    {
    };

On second thought, I guess I can understand why you want to give the
user the ability to package the mapping into container-hood and hand
it to you.  This reminds me of the kind of explicit concept mapping
you can do in Haskell.  

My feeling is that if you want to go this way, the user should be
supplying a traits *generator*, not the traits themselves.

>> I am a bit concerned about how reference
>> documentation for various components and headers is grouped all into
>> one file, containers.html.  I don't believe the precedent set by the
>> pickling support in this regard was a good one.  We have a
>> semi-coherent [;-)] reference manual organization - why should there
>> be any supported public headers missing from it?  IMO it would be
>> better (though not a showstopper) if containers.html were just
>> narrative/tutorial, with pointers to the hard-core reference material.
>
> Yes, I think this is inevitable, given the weaknesses you've
> identified. I guess I was trying to economize on documentation
> effort,

I can understand the desire ;-)

> but I suppose it needs to be more rigorous.

Unfortunately, yes.  If I can't understand it, it won't survive well
in the BPL codebase.

>> I'd also like to hear if Joel has any concerns.
>
> Me too. Thanks for your comments so far - I'll work on them as part of
> the current updates I'm doing.

My feeling at the moment is that this work is going to be great, but
that it probably won't be ready (or I won't be comfortable with it) in
time for 1.31.0.  We can still try, but I'm going to be busy...

> -- 
> Raoul Gough.

> export LESS='-X'

I read the help on less -X, but I don't understand why I'd want this?

-- 
Dave Abrahams
Boost Consulting
www.boost-consulting.com