Rob Stewart wrote:
From: FlSt@gmx.de
[...]
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