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

Subject: Re: [Boost-users] [range] questions about documentation and usage
From: Jeffrey Lee Hellrung, Jr. (jeffrey.hellrung_at_[hidden])
Date: 2012-11-04 04:02:31

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:
>
> > Please demonstrate what you would consider an improvement. I think
> > many of us have tried to obtain something from you to help us. I
> > appreciate that negative criticism is to some extent useful, but you
> > aren't giving me clear enough suggestions for positive corrective
> > action.
>
> I pointed out things that I thought were wrong and asked to what I was
> missing.  The feedback that I got back was that no one else thinks
> anything is wrong and don't see my point.  From my stand point,
> I'm just pointing out mistakes.I didn't think of it as negative criticism.
> The feedback I've gotten has convinced me that I didn't get any
> thing wrong

I'm not sure how you came to that conclusion...

> and that in fact my observations point to real mistakes
> in the documentation

Deficiencies, yes, but I don't think you've pointed out any mistakes.

> 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. Is the
documentation under [1] and/or [2] unclear?

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

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 express
> and
> X::iterator as associated types.
>

That's the option described by [1].

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

d) Each function template documentation page should describe the
> concepts of it's template parameters.
>
> the range find function described here:
>
> http://www.boost.org/doc/libs/1_51_0/libs/range/doc/html/range/reference/algorithms/non_mutating/find.html
> refers to a type "range_return_value" which I can't find anywhere.
>

Valid criticism. Go up a couple levels in the documentation and you might
find [4].

> I'm guessing that the type Value can't be any type but is likely restricted
> to
> some type related to SinglePassRangeConcept?  I don't know what it is.
>

A reference to existing documentation on std::find would be the
easiest/cheapest way to address such documentation deficiencies; AFAIK,
boost::find is implemented directly in terms of std::find.

A function template page should look more like this example:
>
> http://rrsd.com/blincubator.com/function/
>
> Note that this includes a small example which can be compiled.
> Note it should contain the header file to be included.
>

That's a good model.

> e) A type or class template documentation page should have a little
> bit more information.  Take your example:
>
>
> http://www.boost.org/doc/libs/1_51_0/libs/range/doc/html/range/reference/utilities/iterator_range.html
>
> which isn't too bad.  But it would be helpful if it included the following:
>
> Template parameter.  I can guess that the "ForwardTransversalIterator"
> parameter should model the ForwardTransversalIterator concept of
> the iterator library.  But it should be stated explicitly.
>

Agreed. And, actually, the documentation states that the template parameter
actually need not satisfy the Forward Traversal concept, but then only a
subset of the documented interface is available. What that subset is,
though, is not explicitly documented, I think :/

> Model of: SinglePassRange concept
>
> Valid Expressions (AKA member functions) should state the
> concepts (type requirements on member function template
> parameters.  For example, if I see:
>
> template< class ForwardRange >
> iterator_range& operator=( ForwardRange& r );
>
> It's not clear what type I can plug into ForwardRange.  Can
> I use any type which models SinglePassRange like this:
>
> std::vector<int> x;
> boost::range::iterator_range<std::vector::iterator> v(x);
>
> or do have to use another iterator range as an argument?
>
> I can't tell from reading the documentation.
>

The documentation on boost::iterator_range is, indeed, incomplete. I think
it has been around for quite some time and hasn't seen much love for a
while :/

> Here is a model for a class template page
>
> http://rrsd.com/blincubator.com/type/
>
> ====
> 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]),
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".

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

> 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? Seems like a perfectly legitimate way to control ADL. I know this
technique is used for the operator| overloads of the Boost.Range adaptors.
If this is used also for boost::find (defined as boost::range::find and
brought into the boost namespace by a using declaration), it's likely to
avoid boost::find being found by ADL.

Whether boost::find should really be boost::range::find is another issue.

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

> >> The documentation could always use more
> >> examples. I did put these into a separate area in the documentation
> >> because I wanted to make them all have tests. The change of layout
> >> might not have been optimal and I'll think about this.
>
> The examples I'm refering to are not full blown tutorial type examples
> (which are helpful but a lot of work to make).  But rather 5-15 lines
> mini-programs written at the time the documentation is written.  They
> are not hard to write when the code is being developed and very
> helpful to users of the documentation.
>

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

- Jeff

[1]
http://www.boost.org/doc/libs/1_51_0/libs/range/doc/html/range/reference/extending/method_1.html
[2]
http://www.boost.org/doc/libs/1_51_0/libs/range/doc/html/range/reference/extending/method_2.html
[3]
http://www.boost.org/doc/libs/1_51_0/libs/range/doc/html/range/concepts/single_pass_range.html
[4]
http://www.boost.org/doc/libs/1_51_0/libs/range/doc/html/range/reference/algorithms/introduction.html