[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:
>
>> 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?

The container_proxy is a container wrapper, and it returns element
proxies that do the range checking - they cooperate on tracking
element movement due to inserts/deletes. However, container_proxy is
completely optional and only works with vector-like containers anyway
(e.g. deque works, list doesn't).

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

I don't think it can be the default, since it isn't embedded in the
suite. It provides an interface which is compatible with the indexing
suite, but is also designed to be reasonably convenient to use
directly in C++. This is important, since there might be C++ code that
modifies the container while Python code still holds proxies pointing
into it.  This was one of the issues I raised early on with the
previous version of the suite, since the proxying was actually hidden
from the C++ code. As I said then, the ideal solution is to use a
container of shared pointers anyway - if that's not possible, exposing
a container_proxy wrapper of the container is an option for
vector-like containers.

[snip]
>>> 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.

OK - will change this.

[snip]
>> 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.

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

Thanks for this - I wish I'd seen that when I first started reading
the Python docs. By the way, I'm pretty sure the C++ standard never
uses the word "Concept" in this context. It refers to "requirements"
like "LessThanComparable" or "Assignable", which is more or less the
same thing but with a different name.

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

I couldn't think of anything better at the time. It's just a hook to
let the algorithms, container traits or value traits do any additional
set up during the def() call for the whole suite. Maybe def_hook?

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

... can correctly deduce /for STL-like containers/.

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

That probably explains the troubles. I'll look into using the
value_type instead.

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

It should ideally support by-value element removal, so set is the
right abstraction. Are you suggesting that it isn't fully functional?

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

Yes, I don't see how doing it via specializations for each individual
property would make things more convenient for the client code. I
guess I'll document the available base classes that perform the
automatic deductions when I provide separate HTML files for each
header. This makes it more or less trivial for STL-like containers,
without actually requiring it.

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

Makes sense.

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

OK - there's a fair bit here to chew on.

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

Here's the answer:

    Disables sending the termcap initialization and deinitialization
    strings to the terminal.  This is sometimes desirable if the
    deinitialization string does something unnecessary, like clear-
    ing the screen.

every GNU-based system I've ever worked on clears the screen by
default on exit from less. Annoys the hell out of me, when I've just
scanned a man page to the information that I want, hit "q" and then
the information disappears! What morons! :-)

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