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 15:30:31

On Sun, Nov 4, 2012 at 10:10 AM, Robert Ramey <ramey_at_[hidden]> wrote:

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

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

IMHO, a strict reading of [5] does not necessitate that a concept provide
ways to allow the user to create models (e.g., write archetypes). Indeed, I
can imagine situations where all models of a concept are provided entirely
by a library (see Boost.Parameter's concepts). I could be wrong on the
intent of concepts but this seems to jive with the concept documentation of
Boost.Range, Boost.Fusion, and Boost.MPL. They all have extension
mechanisms you need to go through to adapt UDTs to the concepts documented
by the respective library.

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

Then...why are you assuming that?

 In any case, these should be part of the
> "valid expressions" section of the SinglePassRange concept.
>

*That* would be a mistake; begin/end member functions and an iterator
typedef are just sufficient for a type to model SPR, but they aren't
necessary. E.g., std::pair< int*, int* > is a valid range.

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

"totally incomprehensible" is unnecessary hyperbole, and is not helpful
regarding improving the documentation of this section. There's only a
couple hundred words there. Come back with something concrete.

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

For the record, I do not.

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

No spelunking through code is necessary, you did that on your own. If you
want to understand what you can do and how to use a concept, the concept
documentation is sufficient (IMO). If you want to create an archetype or
adapt an existing UDT to a concept, the extenstion mechanism documentation
is sufficient (IMO). This is also how Boost.Fusion and Boost.MPL (at least)
are structured, AFAIK.

[...snip more discussion on what belongs in the concept documentation...]

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

I'm sorry, I'm still not following. Can you be more concrete? I don't know
what you mean by building a library "concurrently with the documentation",
and I'm not sure what your suggestion about "how to go about these things"
actually is.

I snip the immediately following discussion, as it basically boils down to
you asserting that the documentation of a concept ought to allow one to
directly write a model or archetype of the concept.

[...]

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

Hmmm, works for me, as in, it fails to compile.

--------
#include <boost/concept/assert.hpp>
#include <boost/range/algorithm/find.hpp>
#include <boost/range/concepts.hpp>

struct X { };

namespace boost
{
int const * begin(X&);
int const * end(X&);
} // namespace boost

int main(int argc, char* argv[])
{
  X x;
  //boost::find(x, 0); // compiler error
  boost::range::find(x, 0); // compiler error
  return 0;
}
--------

Same error comes up as in a BOOST_CONCEPT_ASSERT, pointing to a line
referencing range_const_iterator and range_mutable_iterator, suggesting
that Boost.Range cannot associate an iterator type with X.

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

Really, a "whole bunch"? It has a single "using range::find" that pulls
boost::range::find into the boost namespace. How can this be any worse than
defining boost::find directly in the boost namespace? It's actually way
better this way as now boost::find will be less likely found via ADL.

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

You're conflating 2 "issues": (a) it's boost::find and not
boost::range::find; and (b) boost/range/algorithm/find.hpp has a using
declaration. (a) is legitimately debatable (I don't have strong opinions,
other than there is a std::find so I can see the rationale for
boost::find). There really is no issue (that I can see) with (b)
independent of (a).

[...]

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

Confirmed now that I've looked at the code.

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

AFAIK, boost::find is documented, and boost::range::find is not. It's an
implementation detail that, as I've said above, is no worse than defining
boost::find directly in the boost namespace.

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

Yes, you have brought up some deficiencies in the documentation. But I
don't agree with many of your alleged "mistakes". Someone with authority on
concepts may agree with you, but AFAICT, the Boost.Range documentation on
concepts is complete and on par with other Boost library documentation on
concepts.

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

- Jeff

[5] http://www.boost.org/community/generic_programming.html#concept