On Wed, Apr 9, 2008 at 11:16 AM, shunsuke <pstade.mb@gmail.com> wrote:
Giovanni Piero Deretta wrote:
I'm attaching some comments after a not-so-brief reading of the egg documentations.
I'm attaching the answers to your egg.txt.
And here are my replies to some of your answers.
=== Introduction
- "Egg what for?" (shouldn't it be "What is Egg for?"): why do I need to use the ugly BOOST_EGG_PIPABLE syntax? why doesn't the following syntax work?:
result_of_pipable<T_plus>::type const my_plus = {};
(As you pointed,) Those macros are unneeded in the case of stateless functions. So, I propose a new `static_` form:
The adapting form in review version: result_of_curry2<F>::type const c = BOOST_EGG_CURRY2({}); // stateless is changed to a new `static_` form *without macros*: static_< result_of<T_curry2(F)> >::type const c = {{}};
How do you think about this new form?
Much better, but I do not particulary like the static_ name (expecially because of the trailing '_'). Statically_initialized is too long, so if you can't come up with anything better and shorter, I can live with static. Btw, can you explain me (again, please :P) why result_of<T_curry2(F)> >::type const c = {{{}}; isn't enough? I guess I should see static_ implementation. What about collapsing static_ and result_of in a single class, and allowing arbitrary composition: apply<compose, apply<compose, my_fun1, my_fun2>, my_fun3>::type c = {{}}; The apply name is already taken by MPL, but maybe something of the sort.
- Notation:
Please get rid of this notation. The double underscore syntax hurts my eyes :). Also it is very hard to have to always refer to this section when reading the documentation. The last four entries are obvious enough, and so is decltype. For the rest you should find a different, self explicative notation.
Because the document of FunctionAdaptor section is going to be simplified, I think I can remove most of them.
Great!
3. "Unintentional ADL". It is claimed (in the solution) that egg solves the problem of unintended adl by making make_filtered an object.
Be aware that gcc performs ADL even on function objects.
Which version of gcc?
AFAIK all of them: namespace a { struct foo_t {}; struct bar_t { void operator()(foo_t){}; } bar; }; int main() { a::foo_t foo; bar(foo); } This compiles at least with gcc-4.1.3 and gcc-3.3.6 (it doesn't with comeau online) The biggest problem is of course is that gcc fails to compile this: namespace a { struct foo_t {}; struct bar { }; }; void bar(...); int main() { a::foo_t foo; bar(foo); } I.e. ADL finds even non callable entities! Comeau online compiles it cleanly.
- "Binding functions"
"boost::result_of isn't used for now". Why?
Because of a bug described in the `egg::result_of_` section.
Ah, now understand why I'm missing some documentation details! the documentation doesn't always clearly show which words are links. for example in my browser result_of_ is not visually distinguished from the rest of the paragraph. I do not know if it is a problem of my browser of you need to fix your CSS.
The description of lazy supposes that the reader is familiar with boost lambda which is a non trivial library. Of course egg is not exactly a beginner library, so it might be expected.
The visual distinction between make_filtered and make_Filtered might not be caught immediately. May be change the name? Also, I do not like this convention of having the lazy variant having an uppercase letter in the middle. Is this just a random example or is meant to be a useful convention? (BTW, I think that egg documentation should have a good section about function object naming conventions).
I don't find a convertion for lazy and unlazy functions yet.
I do not either, other than puting them in a 'lazy' namespace. This is one of the reasons I like optionally lazy function objects :).
Also I do not like the T_* convention to name function types. I liked best the old op_* convention.
LexicallyTypedObject concept is not restricted to FunctionObjects. "T_" is short of "typeof". This concept is a workaround for "missig decltype" in C++03.
What about using the prefix 'typeof_' or a postfix '_type'? I really dislike that uppercase T. Maybe even a postfix _t would be more than enough.
- Getting initializers
"As mentioned before, the static initialization is important". Why? You still do not explain it.
Click "before".
Right, missed the link. Probably this part should go in an advanced section or at least an had-hoc section on static initialization. Certainlin not in the quick start.
- Object generators
It is completely un-obvious what are you trying to explain here. Even clicking on the link about object generators doesn't really enlighten me much: if the name 'always' parallels the mpl counterpart it should be a function that always returns the same value. But why you call it 'always_return' and not just 'always'?
Ah, I see, you later name the actual object 'always'. But why the adapted class (or would be better to call it 'facaded'?) was postifixed _result? If it were just a metafunction to compute the result type I would understand, but the 'call' method makes it something more than an mpl metafunction.
always_result means a return type of always.
sure, but it also contains the body of 'always' (i.e. the call member), so it is a misleading name.
"bind expressions my be less readable. nestN offers a better syntax"
egg::nest2(plus)(5, egg::nest2(_1_(_1))(_1_(_2), _0_(_1))) (i6)(minus, i7);
Is that a joke?!?!? :) I use both boost.bind and lambda.bind extensively and while I might not like the syntax, at least is clear what it does. the _N_(_M) syntas not only is *extremely* ugly, but I have absolutely no idea of what are you trying to do! Please explain.
The haskel-like lambda syntax doesn't help. I can barely read simple haskel expressions, certainly not nested lambdas. I expect most C++ programmers (even very sophisticate ones) wouldn't either.
nestN returns a function which returns a function which returns a function... Probably I should change the notation into C++0x lambda expression?
That would probably help a little. Much more prose (like a step by step analisys of what that expression means) would be much better.
I find the prefix T_ extremely ugly. I preferred what I think egg used previously, i.e. the op_ notation.
This convention is not restricted to FunctionObjects. `T_` is short of "typeof". This convention is a workaound for "missing decltype" in C++03.
I know, I just find the syntax ugly.
Even better would be to segregate function object types to their own namespace, a-la fusion or proto. (i.e. the functional namespace).
See this:
namespace poost { namespace op { struct foo {}; }
op::foo const foo = {};
namespace nested { namespace op { struct bar {}; } op::bar const bar = {};
void test() { op::foo my_foo = foo; // doesn't compile } } }
Thus, I've rejected segregated-namespace-way.
uh? ... void test() { poost::op::foo my_foo = foo; // it compiles! } Seems a very simple fix (and arguably the right thing in the first place!)
- Pipable and Ambi: These concepts are intermixed and too fuzzy IMHO. All the details of Ambi and Pibable, should be collapsed on a simpler Pibaple concept
I made some efforts to define these concept. I believe this way is the simplest (for now).
(and BTW, Ambi is not a self descriptive name):
I will be happy if a better name for ambi is found in this review.
" A pipable function object 'f' is an unary polymorphic function object for which the expression 'x | f' is equivalent to f(x) ".
Making a function object pipable should just be a matter of deriving it from an (empty) egg::pipable, which would manifest the intention of the object to participate in pipelines.
I'm not sure it has an advantage over higer-order functions.
Less intellectual overhead :). I need to know less things to appreciate what 'pipable' means. Concept should be as simple as possible IMHO. (you can of course add "... but not simpler" :) )
Oven should provide a free function operator| (that will be found via adl, and sfinae'd on is_base_and_derived<pibable, Rhs>).
Sorry, I couldn't understand this sentence. Why is Oven stated here?
I meant Egg of course. What I wanted to say is: Egg provides a templated operator| in egg namespace. There is a single implementation for this operator and is: namespace pibable_ { struct pipable {}; // ADL hook template<typename Lhs, typename Rhs> typename boost::lazy_enable_if<bost::is_base_and_derived<pipable, Rhs>, boost::result_of<Rhs(Lhs&)> >::type operator|(Lhs& lhs, Rhs rhs) { return rhs(lhs); } template<typename Lhs, typename Rhs> typename boost::lazy_enable_if<bost::is_base_and_derived<pipable, Rhs>, boost::result_of<Rhs(const Lhs&)> >::type operator|(const Lhs& lhs, Rhs rhs) { return rhs(lhs); } } Any unary function object that wants to model the pipable concept must derive from pipable_::pipable. This will trigger ADL and will find the operator | when necessary. (In c++0x of course you would put operator| in the global namespace and make it a template function constrained on the (non auto) concept Pipable) This of course only work with unary callable entities. to make a non-unary entity unary, apply the appropriate curry/bind grease :). This will also get rid of the need for Ambi. As an extension (I'm not proposing it, it is just for the sake of discussion), '|' could be a synonym for compose (IIRC it is spelled '$' in haskell): if lhs and rhs are callable entites (in practice you detect pipability): a | b is the same as: compose(b, a); else if only b is a callable entity: a | b is the same as compose(b, always(a)); else the expression is illegal. [IIRC in haskel values are in practice treated as nullary functions, so the use of 'always' here would mimic the functional comunity usage] This means that you can create pipelines of transforms: map(my_range, my_first_view|my_second_view) Would be the same as: map(my_range, compose(my_second_view, my_first_view)); or map(my_range, my_first_view) | protect(lazy(_1, my_second_view)) [where map returns a pair of transform_iterators]. As an alternative for the ugly protect + lazy, read further.
Adapting function objects of arbitrary arity to make them pibable is really orthogonal. Lambda would works just fine, but egg could provide (if already doesn't)adaptors to add curriability functionality to a generic function object:
struct op_foo : curriable<op_foo> { typedef whatever result_type; template<class A1, class A2, class A3> result_type operator()(A1, A2, A3); } foo;
x | foo(_, 10, "bar");
Note that you need to explicitly mark the missing argument. And in principle you can have more than one missing argument:
f(_, 10, _)("baz", "bar");
Sorry, I couldn't understand this proposal. What does `x | foo(_, 10, "bar")` mean?
hum, let me see, in egg syntax it should be: compose(lazy(foo)(_1, 10, "bar), always(x)) but see below:
Also, f(_, 10, _)("baz", "bar"); seems supported by egg::lazy.
yes, the only difference is that the result is not a lambda expression (as if there was an implicit protect): see the difference between: lazy(foo)(lazy(bar)(_1, _2)); // ll::bind(foo, ll::bind(bar, _1)) and: lazy(foo)(protect(lazy(bar)(_1, _2))); // ll::bind(foo, protect(ll::bind(bar, _1))) with my syntax (actually this is lifted directly from the generalized currying in FC++), you could spell the latter: foo(bar(_,_)); // s this is important if egg were (as one would expect) to register its operator| with boost::lambda: map(my_range, _1 | lazy(is_less)(_1, 10)); // does not work! map(my_range, _1 | protect(lazy(is_less)(_1, 10))) ; //ok map(my_range, _1 | is_less(_, 10)); // also ok A nice name for the generalized curry operation is of course curry: auto is_less = curry(is_less_non_curriable); assert( is_less(_, 10)(7) == true ); This is trivially implementable with lazy + protect, but an ah-hoc implementation might be simpler and easier on the compiler (no need for full blown lambda support). Also, i spell the missing parameters '_' because that's what FC++ used, 'deferred' might also be a good name. What do you think?
- Major Function Object: this name is not descriptive. Make it Lambda Compatible Polymorphic Function Object.
A specific name should not be contained in this concept name so that MajorFunctionObject can support yet another lambda library.รน
Hum, the support hook shuld only be boost::result_of and its protocol.
Or better, modify boost.lambda so that any Polymorphic function object work with it (shouldn't be hard), so that we do not need an ad-hoc concept.
It seems hard.
I do not think so, in fact I think that there are patches around. On the other hand, I'm not the one doing it, so I shouldn't complain.
- Static Function Object: again, you are mixing a concept, with pertains types, with restriction on object usages or initialization. I do not think that you can encode the fact that 'f' is statically initialized in a concept. Also, this 'concept' badly need a rationale!
I'm going to modify this concept. The "stateless" requirement will be added, and the static-initialization guarantee will be removed. Do you think it is right?
I do not know if it is right, but I have only needed static initialization for stateless function objects. Others might have different experiences (and they should better speak now ;) ).
- Writing you strategies: why should I want to write my own strategy? What can I do exactly?
See the Eric's request below.
In valid expression you introduce apply_little, but you do not document it anywhere it seems (its use seems obvious, though).
It isn't placed in Valid expressions table?
yes, my fault, but those tables are almost unreadable to me, sorry :(
Where is the custom forward strategy in the valid expression box? How should be interpreted.
Sorry, I couldn't understand this question.
Me neither :). I have no idea of what I was asking.
=== Function builders
- function: finally we find the function class! I object in another class in boost called function. What about function_adaptor?
This is not an adaptor.
Hum, IMHO,yes. It is just adapt a class from the internal Minor function object convention to the external Major function object.
IIRC, Boost.Phoenix also uses "function".
Yes, I saw it too. But Phoenix is not a first class boost library yet :). when it will be reviewed, I'll make the same comment.
BTW, do you want to encourage stateful function objects? Since I learned 'bind' I have never again written a stateful function object.
I rarely use Boost.Bind and Boost.Lambda in production code.
Why not? I do all the time.
It would be great if egg had a way to express the result type of bind (in fact I think that lazy does exactly that).
If a Boost.Lambda expression is simple, it is feasible. The full support would be challenging work.
Agree. It would enought it just supported function composition (i.e. no overloaded operators).
"it can be considered a port of callable". I do not know what 'callable' is. Please provide a link/reference.
Click 'callable'.
Ah, I see. Again, the word here doesn't show up as a link untill i mouse over it.
- implicit. "Implicit adds implicit conversion support to a cast from function". What does that means? What are the use cases?
It automatically passes initialized types to cast functions.
could you provide an example?
Arguments except for the last one are captured by-copy. When you need to capture by-reference, pass arguments using boost::ref." Why the last one is an exception (I guess your use case, and mine too, is to pass ranges and containers as last arguments)
No, it is not range problem. Currying is "binding", per se. It must copy arguments to make an intermidate FunctionObjects. The last argument is not needed to be copied fortunately.
I figured it out later :).
In the example, mabye 'my_minus_base' would be a better name for the struct?
I prefer prefix, but I'm sure I don't know english well. :-)
Oh, well, me neither :P
- Uncurry. Uh? What is this useful for? BTW, a more useful facility would be the ability to traverse all objects stored in a curry closure (is closure the right term?), like the undocumented boost.bind apply_visitor.
I couldn't find boost.bind apply_visitor.
The actual name is visit_each (which I think is the protocol used by apply_visitor). There is some reference in bind/storage.hpp. The actual api is not documented, but I guess that the intent is that boost::visit_each(vistitor, bind-expression); should iterate on all arguments closed in the bind expression. Searching the boost mailing list reveals that bind_visitor has been broken for a while. I do not know if it is still true (I have never used it - yet).
- lazy. Useful, 'nuf said. I would also like to have optionally lazy function objects, but it seems that I'm the only one who likes them :).
In fact, I know another one who likes them.
:)
- mono. I have never needed this, but I understand that it might be useful sometimes.
IIRC, Dan Marsden's Boost.Traversal makes use of one.
I was missing the fact that it allows one to inspect the argument types.
- nestN. This is still very very obscure to me. The more I read the description the more confused I become. I understand that the problems are nested lambda expressions, but I do not understand how your soultion work, what the syntax is and why it must be so complicated
A comparison between "bind" and "nestN" has been requested, so I will add some explanations of details.
Thanks.
(did you see how phoenix handles the problem via local variables to lambda?)
Because Egg is not "yet another lambda library", I'm not sure it should provide some cool syntax.
well, at least readable :)
If reviewers regard it as an inadequate component, I will simply remove nestN from Egg. BTW, how can we write nested lambda using phoenix?
lambda[ for_each(_1, lambda(_a = _1)[ front(_a) += _1] )(range) [at least It should work, given appropriate definitions of for_each and front]
- bll_N. Is this just an alias for lambda placeholders? why i can't just use _N and boost::lambda::placeholderN_type?
bll_N is a model of LexicallyTypedObject so that you don't need to remember the name "placeholderN_type".
so now I have to remember two types instead of one :)
- construct_braced{1,2}. Eh? what is this for?
- construct_variadic. Same as above.
egg::generator needs this to initialize pod types.
Is it a general purpose utility of just an implementation detail?
Doesn't having to specify the signature partially defeats the purpose? Why can't I just do:
BOOST_AUTO(f, unfuse(fuse(_1 + _2)))
and keep my lovely polimorphic-nes? i.e. would it be possible for egg to (optionally) register all its type with boost.typeof?
Egg could do, but I think it is nearly impossible for Boost.Lambda.
Unless lambda registers its types of course. Anyways, It would be enough for me if this worked: BOOST_AUTO(foobar, compose(foo, bar));
Of course the user would be responsible for registering its own function objects.
I can't imagine users register their function objects.
Well, if an user wants to use typeof it needs to registers its types anyway. Enough for now. More comments to come. -- gpd