On Sun, Nov 4, 2012 at 10:10 AM, Robert Ramey
<ramey@rrsd.com> wrote:
Jeffrey Lee Hellrung, Jr. wrote:
> On Sun, Nov 4, 2012 at 1:37 AM, Robert Ramey <ramey@rrsd.com> wrote:
>
> Neil Groves wrote:
>> On Sat, Nov 3, 2012 at 11:29 PM, Robert Ramey <ramey@rrsd.com> 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