Re: [boost] [egg] some comments

From: shunsuke (pstade.mb_at_[hidden])
Date: 2008-04-09 05:16:44

Giovanni Piero Deretta wrote:
> I'm attaching some comments after a not-so-brief reading of the egg
> documentations.
> 
> I've started the review a while ago, so some of the comments
> (especially those about static initialization) might be obsolete after
> the long discussion I had with the author in another thread, but
> Shunsuke Sogame might want to respond again for those that missed that
> thread.
> 
> This is not yet a complete review, I want to spur further discussion
> on this library (especially on the documentation) before providing a
> final review.
> 
> Finally I would like to encourage others to review this library (no
> reviews yet and we are in the second week!) because it provides some
> extremely needed functionalities.

I'm attaching the answers to your egg.txt.

Regards,

-- 
Shunsuke Sogame

> === 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?

>   Also, it seems that egg takes a lot of care to make sure that
>   function objects are statically initialized, but they can't still be used
>   in headers because of ODR. Egg should provide a solution to that (more to come).

I should consult the author of Boost.Proto.

> - 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.

> === Quick Start
> 
> - "Problems of function templates"
>    
>   "take by reference" shouldn't it be pass by reference?

Will fix.

>   1. "a function template is not an object". Provide an example of the
>      prolem. Showing how hard is to use 'make_filtered' with bind
>      would be nice.

Ok.

>   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?

> - "Binding functions" 
> 
>   "boost::result_of isn't used for now". Why?

Because of a bug described in the `egg::result_of_` section.

> Higher Order functions.
> 
>   I do not see how 'pipable' is an higher order function. Here
>   it is just a function adaptor.  In fact make_filtered is already an
>   higher order function (i.e. a function that takes/returns another function
>   as a parameter/return value, in this case the filtering predicate).
> 
>   Ah, I see, you mean that pipable takes a function and returns
>   another function which is pipable. The intent seems lost in the
>   MACRO_NOISE.
> 
>   Anyways, the section name is misleading. Just call it pipable
>   functions. BTW, 'pipable' really sounds wrong to me (but I'm not a
>   native english speaker, so YMMV :) ).
> 
>   If you rename the section, you should of course put the paragraph
>   about lazy on its own section. 

Ok.

>   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 hope the authorities help me.

>   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.

> - Getting initializers
> 
>   "As mentioned before, the static initialization is important". Why?
>   You still do not explain it.

Click "before".

> Also, what if I want to put my funciton
>   object in an header file and do not want to risk ODR violation?.

Should we state that "Use EGG_CONST everywhere." ?
Anyway I should consult Eric.

> - 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.

>   I completely fail to see the parallel with object generators.
>   What kind of magid does exactly generator do?

Probably I should write an example which compares egg::generator with
a hand-made code.

>   "function_facade might remind you of boost::iterator_facade". Yes, so what?
>   You do not explain how it works here. Why should I prefer
>   function_facade to poly? What are the advantages of either
>   solutions? 

poly builds a stateless POD function object type,
whereas function_facade cannot be POD, but can have user-defined constructors.

> - Partial application.
> 
>   How partial application described here is different than the one
>   presented in 'lazy'? (except that as far as I understand, 'lazy' is
>   implemented in term of nestN).

The introduction of 'lazy' might be too early.

>   "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?

>   Anyways, this certainly shouldn't be in the quick start section (if
>   it has to be shown at all and not remain as an implementation detail
>   of the library).

Ok.

>   You say that curryN can be implemented in term of nestN (?), but you
>   do not explain what it does.

It seems bad that I am using haskell notation.

> Fusing
> 
>   Why should I use egg fusing/unfusing instead of the fusion provided
>   ones (do they have other benefits/functionalities?

Egg can support stateful static-initialization.

> === Concepts:
> 
> - Lexically Typed: This is not really a concept (how would you express
> in ConceptC++? Can you write a C++03 concept check?). It is just a
> convention.

So it seems.

> 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.

> 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.

> - 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.

>  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?

> 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?
Also, f(_, 10, _)("baz", "bar"); seems supported by egg::lazy.

> - 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.

> 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.

> - 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?

> - Little Function: initially I didn't like this concept, but now I see
>   its utility. Still, the name isn't descriptive. What about Primitive
>   Function Object?

I will consider it.
BTW, it was originally called BabyFunction.

> - result of tutorial: this really doesn't belong here. boost.result_of
>   documentation should just be corrected (may be by replacing it with
>   this section).

I agree. Boost.ResultOf is nearly undocumented.

> === Forwarding strategies
> 
> - What are these for? Provide rationale and use cases in the
>   introduction.  It is very hard to follow the expressions in the
>   'valid expressions' boxes. In general this whole chapter is very
>   obscure. I think you are trying to be too formal.
>
> - by_perfect: you should explain explicitly what perfect forwarding is
>   (I see the link to 'the forwarding problem', but a small
>   introduction should be necessary here).

I think moderate users don't need to read this section.
But I admit that by_perfect should be roughly explained using an easy example.

> What is 'aI' in the
>   'Notation' paragraph? Ah, I see, is the I'th 'a' in an argument
>   list. I do not understand your juggling of 'a's and 'b's.

This is because by_perfect instantiates both const and non-const overload
function declarations while performing overload resolution.
It might be too formal.

> - by_ref: you say that by ref offers larger arity, you should make it
>   explicit that MAX_LINEAR_ARITY is larger than MAX_ARITY.

Ok.

> - 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?

> Where
>   is the custom forward strategy in the valid expression box? How
>   should be interpreted.

Sorry, I couldn't understand this question.

>   What magic does FUNCTION_PREAMBLE do? Why I need it? It is very
>   ugly.  

Mainly because it makes egg::function be a model of PolymorphicFunctionObject.
If interested in the implementation, see egg/detail/function_preamble.hpp.

>   In the paragraph 'specializing function<>' what is function, and why
>   should I want to specialize it?  clicking on it just bring us back
>   to the paragraph!
> 
>   In general, is specializing the forwarding strategy just worth it?

IIRC, while reviewing Boost.Functional/Forward,
Eric requests a forwarding way which can be translated into:
 
   BOOST_EGG_FUNCTION_CALL_OPERATOR(
     (by_perfect)(by_perfect)(by_perfect)(by_perfect)(by_perfect)
     (by_ref)(by_ref)(by_ref)(by_ref)(by_ref)
  , const)

So, I've added support of user-defined strategy.

> === 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.
IIRC, Boost.Phoenix also uses "function".

> - function_facade. Why should I use function_facade instead of
>   function? It seems that it supports stateful funciton objects. Do
>   they work with 'function'?

If you need user-defined constructors, egg::function is not usable.

> 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.

> 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.

>   "it can be considered a port of callable". I do not know what
>   'callable' is. Please provide a link/reference.

Click 'callable'.

> - generator. I finally *think* I understand what generator does. In
>   practice it helps you write constructors a-la functinal
>   languages. The user must implement the deduction rules via mpl
>   expressions and generator provides the boilerplate code to actually
>   instantiate the object. Still it is not yet clear to me how does it
>   work. Some more examples and prose are wellcome.

Ok.

> - 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.

> - poly. "poly is a port of detail::funciton family which is secretely
>   contained in boost.accumulator". This doesn't really tell me
>   anything. It is at best a footnote and yet is the first line of the
>   description. 

Will be moved to "See also" section.

>   This is Yet Another Way to build polymorphic function objects (the
>   others are 'function' and 'function_facade'). Isn't this too much
>   for such a "simple" need? I appreciate the syntax conveniences
>   provided by 'funciton' and 'poly', but I do not think that the
>   'intellectual overhead' of having to understand the different
>   syntaxes is worth it.
>   (as a side note, I do not really understand what function_facade
>   add, even at the syntax level),  

egg::function builds a stateful pod function.
egg::function_facade builds a stateful function which has user-defined constructors.
egg::poly builds a stateless pod function.

> - static_. "builds a pod function object from a default constructible
>   one". How does this work? The adapted function object must be a POD,
>   so what does static_ adds? What is the magic here? I think that it
>   must be exposed, or else it would be hard to understand when and why
>   static_ is needed.

I'm going to slighly change the definition of `static_`
as StaticFunctionObject concept definition is changing.
I had overlooked the power of `static_` before you pointed.
(I will add more elaborate description.) Thanks, gdp.

> - variadic. As you says, Is just a shortcat to fuse. It is really
>   necessary to add it?

Yes in my experience.
Once the new static_ form is introduced, poly + unfuse would be more cumbersome
without this.

> === Function Adaptors
> 
>   - ambiN. I have already commented on the name and on the purpose of
>     this function on the Ambi concept definition.
> 
>   - Compose. This is what pipealbe should be in my opinion! :)
>     Extremely useful. I can't parse the 'valid expressions' section at
>     all!

Thanks to you, valid expression tables are going to be very simplified.
All the macro expressions will be moved to a small "Stateful static initialization" section.

>     Please, do not use the infix notation in the example section,
>     expecially as you do not have introduced it yet. A simple
>     'compose(increment, decrement)' will be more than enough.

Ok.

>   - curryN "The curry family turn Function Objects into curried ones."
>     Please explain what curry means (not that I know what it means
>     exactly :)).  Which is a shame, because it is a very useful facility.

I will do my (english) best.

>     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.

>     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. :-)

>     The examples simply show the two parameters for curried being
>     passed immediately. It might seem that curry simply offers a
>     different syntax for function calling (which in a sense is true). 
>     Maybe show the classic example of 'add_5' being applied to all
>     elements of a container via std::transform.

I agree.

> - 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.

> - Fix. Now, this is really something that needs detailed description
>   :). Maybe You should start with the problem of writing a recursive
>   anonymous functions. (how can it call itself when it doesn't have a
>   name?). Do not use boost.lambda, at least not in the first example,
>   first show a simple hand written function object (which wouldn't be
>   anonimous, but whatever :P). 

I will consider it.
(In fact, I just wanted to show the power of C++. :-)

>   Why do I need to use 'lazy' in the lambda expression? And what does
>   lazy(_1) means? Ah, maybe boost.lambda doesn't allow invoking
>   operator() on a placeholder (never tried), and this is just a
>   workaround. You should specify it though. 

Ok.

>   Also, is lazy itself 'optionally lazy' (it behaves specially when
>   its parameter is a placeholder?). I do not see anything relevant in
>   lazy description.

No, lazy is not 'optionally lazy'.
Using egg, "lazy function" can be made in one shot.
'optionally lazy' is a little confusing, IMO.

> - fuse/unfuse. Again, aren't the adaptors provided by fusion enough?

It can support stateful static initialization.

> - indirect. Nothing useful (or even useless) to say here :)
> 
> - 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.

> - memoize. An use case would be very useful.

I must learn more to find many examples. :-)

> - 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.

> - 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.

> (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.
If reviewers regard it as an inadequate component,
I will simply remove nestN from Egg.
BTW, how can we write nested lambda using phoenix?

> - perfect. Recently a forwarding library has been added to boost. Does
>   'perfect' add anything?

It can support stateful static initialization and can
be used with Boost.Lambda.

> - pipable. I have already commented on pipable in the concept
>   section. 

> - regular. I understand the need for this, but wouldn't it better to
>   just fix lambda. Not that it would be easy...

I wish I could fix it.

> (Also I think that
>   boost.bind and maybe even tr1::bind has the same problem, is that
>   right?).

IIRC, a FunctionObject Boost.Bind returns is not DefaultConstructible.
I should note it.

> - return_. Does this just do the equivalent of implicit cast?

No. This is a higher-order function which modifies return types.

> - tagged. I have never needed strong typedefs for function
>   objects. What are your use cases? 

See egg/protect.hpp.

> === Function objects
> 
> - adapted_to. Shouldn't this be adapted_from?

It can be regarded as "adapted_function_object_to_base_function_object".

> - always/identity/apply. Obvious and generally useful.
> 
> - 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".

>   In the 'valid expressons' it states 'polymorphic
>   boost::lambda::_N'. What does 'polymorphic' refers to here? 

It means that bll_N is a model of PolymorphicFunctionObject.

> - X_construct. Hum, doesn't boost already has this functionality
>   elsewhere?

AFIAK, no if you need the perfect forwarding.

> - construct_braced{1,2}. Eh? what is this for?
>
> - construct_variadic. Same as above.

egg::generator needs this to initialize pod types.

> - get. I think that fusion is going to provide all its
>   algorithm/intrinsics as function objects in the functional
>   namespace, so this might be redundant.

If so, it can be removed.

> - pack. Is this just like make_tuple but with perfect forwarding?
>   (i.e. no need for cref/ref).

Yes. Also, this is a FunctionObject.

> === Utilities
> 
> - expr. It seems interesting, but the example doesn't show anything
>   significative. What does it exaclty do? Ah, maybe I see it, I was
>   missing the BOOST_AUTO. Is type erasure performed using
>   boost.function?

Yes. boost.function is used.

>   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.

> Of course the user would be responsible for registering its own
>   function objects.

I can't imagine users register their function objects.

> - Infix. I like it! Since I saw it on FC++ I have always wanted to
>   replicate the syntax.
> 
>   ... But I do not like how you implement it. I want explicit
>   conformance. You avoid the problem of having a catch-all operator^
>   by segregating it in the infix namespace, but the using is still
>   very risky. The user should have to use an adaptor (infixable?
>   hopefully not unfixable ;) ) to get to use this syntax. I also want
>   to use it in lambdas, so operator^ should be registered with this
>   library... ah, yes, and the | pipe operator too :).

'infixable' seems cool. I will try it.

> - bll/result_of.hpp Nice, but let's just fix lambda!

I agree. Hence, bll/result_of.hpp is written to be independent from Egg.

> === Configuration
> 
>  nothing to say here.
> 
> === Workarounds.
> 
>  - result_of_.  hopefully result_of can be fixed by the time egg becomes
>    part of boost.

I also hope so.

>  - detect_result_type. Same thing for BOOST_MPL_HAS_XXX_TRAIT_DEF
> 
>  - CONST. I wouldn't call this a workaround. ODR violation can be a
>    real problem. I know that the gcc team is working on linker changes
>    to detect ODR violations. Anyways, I do not see how CONST help with
>    ODR violations. For example, what should I do if I want to put my
>    'filtered' function in an header file? Please provide an example.

Should we oblige the use of CONST?
I should consult Eric and Boost.Proto.

>  - OVERLOADED. Please, describe the problem. I think that when a
>    workaround for a compiler bug is present in the interface, it
>    should be explained to the user what the problem is and how the
>    workaround fix it (for example see the 'dummy<int>' idiom and its
>    explaination in 'enable_if' documentation)...

Ok

>    ... ok, I've read the linked discussion on comp.lang.c++ and I
>    understand that nobody really understand the problem :). So, yes,
>    I'm ok with the current (lack of) explaination.