Re: [Boost-users] [range] questions about documentation and usage

Subject: Re: [Boost-users] [range] questions about documentation and usage
From: Robert Ramey (ramey_at_[hidden])
Date: 2012-11-04 13:10:35

Jeffrey Lee Hellrung, Jr. wrote:
> On Sun, Nov 4, 2012 at 1:37 AM, Robert Ramey <ramey_at_[hidden]> wrote:
>
> Neil Groves wrote:
>> On Sat, Nov 3, 2012 at 11:29 PM, Robert Ramey <ramey_at_[hidden]> wrote:
>
> and perhaps in the library as well.  Here
> are a few constructive suggestions.
>
> a) For each concept in your documentation define a concept
> checking class.  OK you've done this - so far so good.
>
> b) Declare a concept class using the BCCL - you've
> done this too - so far so good.
>
> c) Declare concept archtypes from reading your documentation.
> This is described  in the Boost Concept library documentation.
> This archtype class looks like:
>
> template <class T>
> SinglePassRangeArchType {
>     // examples of "valid expressions" from the documentation go here
> };
>
> OK First red flag - the "valid expressions" are members of type T
> so it's not at all clear to me how to fill this in.
>
> If one wants to create archetypes of the various concepts, I would
> think to read [1] and/or [2] (extending the library to UDTs), both of
> which are linked from [3], the documentation on the Single Pass Range
> concept.

This is the red flag I'm talking about. If you have to refer to another page
it means that the concept documentation doesn't contain enough
information to actually write an archtype - or a class which
models the concept.  In other words, it's not an actual concept.

>Is the documentation under [1] and/or [2] unclear?

It is.

Method 1 provides information I would have expected to find
in the SinglePassRange concept. I'm assuming that the member
functions and types would be members of any class modeling
SinglePassRange.  I say "assuming becaus it doesn't actually
say that.  In any case, these should be part of the
"valid expressions" section of the SinglePassRange concept.

Method 2: I confess the explanation is totally incomprehensible
to me.  I might be able to parse the example were I to invest
the time.

>> On might think to add something like:
>> namespace boost {
>> template <class T>
>> boost::range_iterator<X>::type begin(T &t){
>>     return;
>> }
>> } // namespace boost
>>
>> But that raises a few design questions of it's own.
>> 1) What should go into X?
>> 2) this will match just about anything - which will likely create
>> other design problems.
>
>
> Right, but I can only think that one might think to do the above
> based on an incomplete reading of the documentation.

> It may be a fair
> criticism of the documentation that this pitfall is exists, but it's
> difficult to use all parts of any library correctly without reading
> all relevant parts of the associated documentation.

My comments above illustrate that the description/design of the
SinglePassRange concept  is not comprehensible on its own.
Sounds like you agree with that.  I content that it should be considered
a mistake. The fact that it raises these "other questions" proves that it's 
a mistake.
The whole idea of concepts, functional programming or whatever
one want's to call it is to permit the the composition of components
which can be demonstrated correct when considered one at at time.

If you have to read the whole documentation to understand one
one concept - or spelunk through the code - it's a red flag that
some things are coupled where they shouldn't be.

> In this
> particular case, the documentation on the various range concepts,
> like most (all) documentation on concepts that I can think of, only
> tells you what you can do with a given range concept, e.g., if you
> were to write a generic algorithm. A separate topic is creating
> models of the various concepts, and that's what [1] and [2] describe.

As I said before [1] should be part of the concept.  I don't know what
[2] is.

>> Given that we've decided (perhaps too soon) we want to support
>> boost::range::find(std::vector<int>, 0) we likely want to provide

> a.begin() as a valid expression
> and
> X::iterator as associated types.

>> That's the option described by [1].

It's in the wrong place.

> Of course this will ripple to boost::iterator_range so that we'll need
> template<class T>
> iterator_range {
>     ...
>     T begin();
>     ...
> };
>
> But we already have that. So far so good.

>> I don't follow. What relevance does iterator_range have here, exactly?

I'm trying to demonstrate what I would expect the natural development
of the library would be were it built concurrently with the documentation
as I think is the best way to do it.  It's meant as a constructive 
suggestion
as to how to go about these things.

> ====
>> More or less unrelated to my questions regarding the documentation
>> there are a couple of other observations I came upon while looking
>> into this.
>>
>> a) The example from a another post
>>
>> class my_class {};
>> namespace boost {
>> const int * begin(my_class & b);
>> const int * end(my_class & e);
>> } // namespace boost
>> void test3(){
>> my_class x;
>>   boost::begin(x);
>>   boost::end(x);
>>   BOOST_CONCEPT_ASSERT((boost::SinglePassRangeConcept<my_class>)); //
>> error
>>   // above ASSERT Traps even thoug my class matches SinglePassRange
>> according
>>    // to the documentatino

> Even given your above overloads of boost::begin/boost::end (which is
> not the correct way to extend a range interface to my_class; see [1]
> and [2]),

The SinglePassRange concept page says it should work - if it doesn't
it's a mistake in the page regardless of what any other page in the
documentation says. If in fact it doesn't work then the page
should be changed. That's the essential point of this whole
discussion.

>Boost.Range doesn't presently deduce the iterator types
> from the begin/end overloads as, say, an STL implementation would for
> range-based-for. So there's no way for Boost.Range to know that the
> associated iterator types of my_class are int*'s. There's really
> nothing in the SPR concept documentation directly to indicate *how*
> Boost.Range gets those associated types, so it would seem difficult
> to assert that my_class models the SPR concept "according to the
> documentation".

This whole purpose of stating a concept is to decouple the issue
of "what it requires" from "how it provides what is required". If
the concept makes some assumption about how something like
boost::begin(a) is provided by some type attempting to model
the concept - then the concept is wrongly formulated.  Here
is a test.

a) Take any concept page
b) write some code which declares and/or implements the list
of valid expressions. If you feel you need to look elsewhere
in the manual, the documentation is incomplete.
c) compile it - if it fails, the documentation is wrong.
d) run it - if it doesn't produce the expected result then
the implementation doesn't match the "semantics" section
of the documentation.

The SinglePassRange concept fails the above test as the
"my_class" demonstrates.  Therefore the page is either
incomplete and/or wrong.

>>   boost::range::find(x, 0); // error
>>   // Even the though the assertion above traps, this code compiles
>> }
>>
>> At first I assumed that the second error was due to failing to
>> include the concept checks in the code for the range::find template.  I 
>> looked
>> at the code and saw that they were there so I can't explain why
>> the find doesn't trap with the same assert as above.

> Sorry, not following. Are you complaining that the boost::find call
> compiles, and you think it shouldn't (I agree, it probably
> shouldn't)?

Correct.

if for some type T, BOOST_CONCEPT_ASSERT((SinglePassRangeConcept<T>)) traps
then any function which requires it's parameters to model the concept 
SinglePassRange
should also fail to compiler. This is because the implementation of f
find<T> should include the statement 
BOOST_CONCEPT_ASSERT((SinglePassRangeConcept<T>)).
I see that find<T> does in fact include this.  I can't see why it traps when 
called
directly but doesn't when called from within find.  There's a mistake
in the implementation somewhere - (or maybe a dumb blunder in syntax).

> b) I thought that all the stuff was built directly in the boost
> namespace which I object to.  I see it's really built in boost::range
> (much better) but hoisted up to namespace boost  via a wrapper
> and using declaration.  I have to observations about this:
> 1) I don't think using should every be in any header file.

> Why not?

because now a user is importing a whole bunch of names into
his lookup set without being informed of it.  This can easily
create the situation where a working program can all of
a sudden fail to compiler - or worse change it's behavior
when the following statement is added to the code.

#include <boost/range/find.hpp>

It's especially bad if the name is something like "find"
It can be incredibly time consuming to track down a
mistake like this and shapes the programmers confidence
in using libraries whose code he as not personally examined
in detail.

> Seems like a perfectly legitimate way to control ADL. I know
> this technique is used for the operator| overloads of the Boost.Range
> adaptors.

Another bad idea - but off topic.

>> 2) Any user can hoist those things he wants into the some higher
>> namespace - but he can't go the other way.  That is, once he
>> uses your header, he can't undo the "hoisting".  If you really
>> want to provide this "help" at least make it optional.  I think you
>> could do that by providing an optional header which would
>> hoist all the names.

> Just a guess: This hoisting is an implementation detail, only used to
> control ADL.

obviously to me it's much more than an implementation detail - but
I've already made my case.

> Simple examples for the Boost.Range functions should be easy to come
> by and, agreed, would enhance the documentation.

I started off with some questions about the documentation and
things (as they sometimes do) turned into something more than
that.  I've offered suggestions which I think have been constructive.
I hope someone agrees with them.  I realize that implementing them
would be a lot of work - beyond normal maintainence so I don't
realistically expect them to be incorporated.  Maybe they show
up somewhere - perhaps in the working group looking into
incorporating range into the C++ standard library.

Robert Ramey