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

[Raoul Gough]

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). 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?  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'm concerned that iterator_range may duplicate functionality
> somewhere else in Boost.  An iterator_range class has recently been
> discussed on the main list as part of a library review.

OK - I'll keep an eye out for this and replace it when/if boost gets
its own implementation.

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

I'm not entirely happy with this aspect myself, particularly as it
relates to the absent pop() method which would always need some kind
of by-value policy. I've thought of a clean solution, which would be
to require a CallPolicies "package" that contains typedefs and factory
functions for various different CallPolicies that the suite requires.
For example len_policies, element_policies, pop_policies or maybe a
slightly different level of granularity.

>
> The "Extending and customizing" section isn't understandable, since
> you haven't even introduced the components you're discussing at that
> point.
>
> Is it important to make reference to def_visitor, or is that merely
> an implementation detail?  If the latter, it should be dropped as a
> distraction.

OK.

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

>
> There's no description of what visitor_helper is for.

By gum, you're right. Well spotted.

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

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

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.

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

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

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

>
> 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,
but I suppose it needs to be more rigorous.

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

-- 
Raoul Gough.
export LESS='-X'