Re: [boost] [local] Review (and resignation from review assistant)

Subject: Re: [boost] [local] Review (and resignation from review assistant)
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2011-11-21 10:49:26

On Mon, Nov 21, 2011 at 10:33 AM, Thomas Heller
<thom.heller_at_[hidden]> wrote:
> On 11/21/2011 09:31 AM, Lorenzo Caminiti wrote:
>>
>> On Mon, Nov 21, 2011 at 9:56 AM, Joel de Guzman
>> <joel_at_[hidden]>  wrote:
>>>
>>> On 11/21/2011 10:41 PM, Andrzej Krzemienski wrote:
>>>>>>
>>>>>> - What is your evaluation of the design?
>>>>
>>>>> The library is very well designed.  The function declaration syntax
>>>>> looks
>>>>
>>>> a little foreign at first, but it is very easy to get used to.
>>>>  Furthermore, error messages are usually able to point you to what you
>>>> did
>>>> wrong.  The fact that you can write the body of the function in plain
>>>> C++
>>>> code is a major advantage of this library.  Some have referred to
>>>> Boost.Bind, Boost.Lambda and Boost.Phoenix as essentially being
>>>> equivalent
>>>> to this library, but in none of those libraries can you express the body
>>>> of
>>>> your function in plain C++;  one advantage of this is that errors in
>>>> your
>>>> code result in normal C++ error messages rather than cryptic error
>>>> messages.
>>>>
>>>> Lorenzo, given this comment by Greg, I do have one more suggestion for
>>>> documentation (or did someone already say it?). Given the heated
>>>> discussion
>>>> of whether Boost.Bind, Boost.Lambda and Boost.Phoenix are sufficient to
>>>> render Boost.Local unnecessary, perhaps it would be worth to provide a
>>>> comparison of how my code (not too trivial, and not too complex) would
>>>> look
>>>> in either case. For instance:
>>>>
>>>>
>>>> With C++11 lambdas:
>>>>
>>>> for_each( vec.begin(), vec.end(), []( std::string&  n ){ if(n.empty()) n
>>>> =
>>>> "n/a";} );
>>>>
>>>>
>>>> With Boost.Lambda + Boost.Bind:
>>>>
>>>> for_each( vec.begin(), vec.end(),
>>>> if_then(boost::bind(&std::string::empty,
>>>> _1), _1 = "n/a") );
>>>>
>>>>
>>>> With Phoenix:
>>>>
>>>> using boost::phoenix::arg_names::arg1;
>>>> using boost::phoenix::if_;
>>>> using boost::phoenix::bind; // right?
>>>>
>>>> for_each( vec.begin(), vec.end(), if_(bind(&std::string::empty, arg1)) [
>>>> arg1 = "n/a" ] );
>>>
>>> Better:
>>>
>>>  for_each( vec.begin(), vec.end(), if_(empty(_1)) [_1 = "n/a" ] );
>>>
>>>> With Boost.Local (Boost.Closure):
>>>>
>>>> BOOST_CLOSURE_PARAMS( std::string&  n ) {
>>>>     if( n.empty() ) n = "n/a";
>>>> }BOOST_CLOSURE_NAME(replace_empties);
>>>>
>>>> for_each( vec.begin(), vec.end(), replace_empties );
>>>>
>>>>
>>>> Ok, I am not sure if there are no better ways to write it in each
>>>> variant,
>>>> and I may be biased. I also didn't try to compile the examples. But it
>>>> should give the potential users an overview of options they have.
>>>
>>> Yep. This highlights the verbosity well:
>>>
>>>  for_each( vec.begin(), vec.end(), if_(empty(_1)) [_1 = "n/a"] );
>>>
>>> vs:
>>>
>>>  BOOST_CLOSURE_PARAMS( std::string&  n ) {
>>>    if( n.empty() ) n = "n/a";
>>>  }BOOST_CLOSURE_NAME(replace_empties);
>>>
>>>  for_each( vec.begin(), vec.end(), replace_empties );
>>
>> IMO, I'd be better if the example had more than 1-line of code in its
>> body. In the docs, I have an example to show the different approaches
>> that has 2-lines and I'd still think more than 2 statements would be
>> better:
>>
>>
>> https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_local/Alternatives.html#boost_local.Alternatives.local_functions.examples
>>
>> C++11 lambdas:
>>
>>     std::for_each(v.begin(), v.end(), [&sum, factor](double num) {
>>         sum += factor * num;
>>         std::cout<<  "Summed: "<<  sum<<  std::endl;
>>     });
>>
>> Boost.Local:
>>
>>     void BOOST_LOCAL_FUNCTION_PARAMS(double num,
>>             bind&  sum, const bind&  factor) {
>>         sum += factor * num;
>>         std::cout<<  "Summed: "<<  sum<<  std::endl;
>>     } BOOST_LOCAL_FUNCTION_NAME(add)
>>     std::for_each(v.begin(), v.end(), add);
>>
>> Boost.Phoenix:
>>
>>     std::for_each(v.begin(), v.end(), let(_f = cref(factor))[
>>         ref(sum) += _f * _1,
>>         std::cout<<  val("Summed: ")<<  ref(sum)<<  "\n"
>>     ]);
>
> Why not just:
>      std::for_each(v.begin(), v.end(),
>          ref(sum) += cref(factor) * _1,
>          std::cout<<  val("Summed: ")<<  ref(sum)<<  "\n"
>      );

(An open parenthesis "(" is missing before "ref(sum) ...", correct?)

Because I took your suggestion below too literally. I will make this
simplification in the docs (BTW, as I asked in the thread below, if
you or anyone else see any other improvement to the Phoenix, Lambda,
etc code that I present in Local's docs, please feel free to point it
out).

-- from: http://lists.boost.org/Archives/boost/2011/03/179294.php --
On Sun, Mar 27, 2011 at 11:35 AM, Thomas Heller
<thom.heller_at_[hidden]> wrote:
> On Sunday, March 27, 2011 05:43:39 PM Lorenzo Caminiti wrote:
>> Yes, but the use case would be to have factor const *only* within the
>> "function" passed to for_each while keeping it mutable in the
>> enclosing scope. Therefore, programmers have to declare factor
>> not-const within main() and I was wondering if there was a way using
>> Phoenix to add the const only locally within the "function" passed to
>> for_each (this is done by Boost.Loccal using "constant-binding" as in
>> `const bind& factor`).
>
> Oh right, you can do that ...:
>
> let(_a = ref(factor))[...]; // <-- bind as non-const reference
> let(_a = cref(factor))[...]; // <-- bind as const reference
>
> Error messages are a mess currently ... but can be improved. Consider it as a
> bug.
-- end --

Thanks.
--Lorenzo