From: Rob Stewart (stewart_at_[hidden])
Date: 2005-08-07 16:03:36
> I uploaded a new version to the sandbox file vault
> (junction/boost_junction_v10.zip). There was a bug in my
> detail::binary_op_type struct. I removed it ;-) Now all my test programs
> give the same results as Rob's implementation. An now it compiles faster .
I grabbed v11 which you posted by the time I looked. This is the
first time I've studied your code and I have some comments:
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
1. Aside from the junction type in the base class, plus naming
and stylistic differences, your library looks a great deal
2. You don't have iterator range support yet.
3. Your detail::do_comparison() functions would be clearer if
named "compare," don't you think?
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.
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.
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.
7. Your junction class template parameterized with the various
junction classes is an interesting approach. It allows you to
select the operation based upon the junction type while
keeping the operators strictly in terms of that class
8. I think its interesting to use member operators to handle the
junction on the LHS plus the non-member operators to handle
the non-junction on the LHS/junction on the RHS case. That's
possible for your implementation because you do everything
based upon the junction type.
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).
10. Your param_type nested typedef is nice. I had to switch to
taking everything by reference to const to avoid having to
create excessive numbers of templates.
11. You should name the "Range" template parameter "FwdRange" or
"ForwardRange" to clarify that it needs to model the
ForwardRange concept from Boost.Range. 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.
It's clear that you've adopted many ideas from my mails and
library and you've brought many innovative ideas of your own.
-- Rob Stewart stewart_at_[hidden] Software Engineer http://www.sig.com Susquehanna International Group, LLP using std::disclaimer;