From: Florian Stegner (FlSt_at_[hidden])
Date: 2005-08-08 06:43:29
Rob Stewart wrote:
>From: FlSt_at_[hidden]
>
>
>[...]
>
>0. Your *junction classes need to be in your junctions
> namespace. I know Pavel questioned why they weren't in the
> detail namespace, but it is reasonable to think that some will
> want to create a variable to hold such an object for repeated
> comparisons.
>
>
Thats a good point. Hmm, but how should a client declare them?
set<int> a;
junction< disjunction< set< int > > > a_junction = all_of( a );
looks a little confusing. I would prefer something like
disjunction< set< int > > a_junction; But this is not compatible with my
junction<> - class. Do you have an idea?
>1. Aside from the junction type in the base class, plus naming
> and stylistic differences, your library looks a great deal
> like mine.
>
>
It was a little funny: After I fixed my compilation time problems I
looked at your implementation and saw that it was very similar to mine.
Then I tried to bring the best ideas of both implementations together.
>2. You don't have iterator range support yet.
>
>
This will be the next point I will work on.
>3. Your detail::do_comparison() functions would be clearer if
> named "compare," don't you think?
>
>
Good idea. I don't remeber why I called them so.
>4. I see that you've made good use of implementing one type in
> terms of another. I did that early on in my operator
> intensive version and ignored it after refactoring my
> library. I need to revisit that.
>
>
That was the thing which makes the 10% runtime differences between our
implementations without optimizing.
>5. Your n_m_junction do_comparison() tests against count <= M in
> the return statement unnecessarily. The loop is terminated
> with a return if count ever exceeds M.
>
>
You are right.
>6. Do you like | better than ^ as the user-predicate delimiter?
> I didn't even examine the operator precedence when selecting
> one. ^ is higher than |, but both are higher than && and ||.
> That will force the use of parentheses in many expressions.
> Perhaps we should consider the comma.
>
>
Ok you are right, when you say it's important to examine the operator
precedence. But the comma operator looks confusing:
all_of(a) ,equal(), any_of(b).
From the readability viewpoint I like the bitwise-or operator | more
than the bitwise-xor operator ^.
But now I'm a little bit confused. The precedence must be higher, or I'm
wrong?
For example someone writes: any_of( a ),equal(),all_of( b ) && any_of(
c ),equal(),all_of(d); So all_of( b ) && any_of( c ) will be evaluated
before the ",". If you write any_of( a )|equal()|all_of( b ) &&
any_of( c )|equal()|all_of(d) It will give the correct answer.
>9. You have no operator << support. I figure that folks will
> want to be able to stream these objects if they create
> instances (see item 0).
>
>
I removed them, but it's no problem to add them again. I thought, there
was no need for them. Ok could be useful for debugging.
>11. You should name the "Range" template parameter "FwdRange" or
> "ForwardRange" to clarify that it needs to model the
> ForwardRange concept from Boost.Range.
>
Ok I aggree with that. Naming is important.
> It would be nice if
> Boost.Range offered a ForwardRangeConcept concept checking
> class so we could use
>
> function_requires< ForwardRangeConcept<FwdRange> >();
>
> in the function templates and
>
> BOOST_CLASS_REQUIRE(FwdRange, boost, ForwardRangeConcept);
>
> in the class templates to help user's diagnose errors.
>
>
I think this is the correct forum to ask for such a feature ;-)
>It's clear that you've adopted many ideas from my mails and
>library and you've brought many innovative ideas of your own.
>Great work!
>
>
Thanks.
Sincerly
Florian