Re: [boost] [local] Review

Subject: Re: [boost] [local] Review
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2011-11-19 18:25:34

On Sat, Nov 19, 2011 at 12:15 PM, Thomas Heller
<thom.heller_at_[hidden]> wrote:
> On 11/18/2011 09:37 PM, Lorenzo Caminiti wrote:
>>
>> On Fri, Nov 18, 2011 at 8:35 PM, Thomas Heller
>> <thom.heller_at_[hidden]>  wrote:
>>>
>>> On 11/18/2011 04:44 PM, Lorenzo Caminiti wrote:
>>>>
>>>> On Fri, Nov 18, 2011 at 1:49 PM, Thomas Heller
>>>> <thom.heller_at_[hidden]>    wrote:
>>>>>
>>>>> Additionally, despite the introductory motivating statement ("Local
>>>>> functions are a form of information hiding and are useful for dividing
>>>>> procedural tasks into subtasks which are only meaningful locally,
>>>>> avoiding
>>>>> cluttering other parts of the program with functions, variables, etc
>>>>> unrelated to those parts.") I can not see that this library solves this
>>>>
>>>> Why Boost.Local does not solve this appropriately? What do you mean by
>>>> "appropriately" in this context?
>>>
>>> It tries do make it easier for users to define local functions in order
>>> to
>>> make their lifes easier so they don't have to clutter their code with
>>> local
>>> functions. This is what i fail to see:
>>
>> Sorry, I don't understand this sentence. Can you please re-word it?
>>
>>> 1) The macros do not even closely resemble any kind of function
>>> declarations
>>> in C++
>>
>> Many have expressed different opinion appreciating instead the syntax.
>> For example (but also other email thread on Boost and reviews):
>>
>> That said, I understand how this is clearly a subjective point and I
>> respect your opinion.
>>
>>> 2) If you just want to write a simple local function, the epi- and
>>> prologue
>>> is longer than the actual function body (this is true for almost all your
>>
>> A part form the two macro names, you don't have more tokens than the
>> result type, parameter declarations, bind declarations, and local
>> function name. IMO, these are all elements that syntactically should
>> be part of the local function declaration regardless of the length of
>> the body because they describe the local function interface and its
>> bindings.
>>
>>> examples). And once the function body exceeds that limit I wonder why it
>>> should have been a local function to begin with.
>>
>> Others have mentioned that it is fine for them to have long functions
>> and local functions especially in FP:
>>
>> -- from: http://lists.boost.org/Archives/boost/2011/02/176653.php --
>>>
>>> In functional programming, there is no difference between defining
>>> lambdas
>>> and functions (except the former cannot be recursive), and it is not
>>> viewed
>>> as bad practice to have functions spanning a couple hundred lines.
>>
>> -- end --
>>
>> But of course I understand if you prefer to structure your code
>> differently.
>>
>>>>> appropriately. I personally think that exposing macros as the interface
>>>>> to
>>>>> program with the library does not lead to a nice and clean design of
>>>>> ones
>>>>> code.
>>>>> Boost.Local is very different from other heavily macro based libraries
>>>>> like
>>>>> Boost.Parameter. It is different, because I would consider Boost.Local
>>>>> not
>>>>> as a infrastructure library but as a library for end users. This being
>>>>> said,
>>>>> i would not like to see application code cluttered with overly verbose
>>>>> calls
>>>>> to macros.
>>>>> As boost is trying to not only push the boundaries of C++ it should
>>>>> also
>>>>> try
>>>>> to set examples of best C++ practice. I would not consider Macro calls
>>>>> to
>>>>> interface with a library not good practice.
>>>>
>>>> I understand. I disagree but I understand your point. I disagree
>>>> because the use of macros in this context saves the user from writing
>>>> verbose boiler-plate code.
>>>
>>> What kind of boilerplate code?
>>> Defining a global function? Most likely no.
>>> Defining a local functor? Yeah, probably, but do we really need that?
>>
>> Yes, the boiler-plate code to define the local functor. And yes, you
>> need that to implement the local function... Sorry but I'm missing
>> something here... Can you please explain what you mean better with the
>> two question marks above?
>
> The question is about what I my point of criticism is all about:
> Why do we need a macro to define a function, that adds no more true value
> other than that it was defined in function scope?

Because using the macros we can program local functions using
statement syntax for their bodies. The value is both (1) it's defined
locally (close to the code that uses the function) and (2) its body
uses statement syntax.

>>> Defining a global functor? This is not possible with Local, so the answer
>>> would be "no" here.
>>>
>>>> IMO, it is similar to Boost.ScopeExit: Instead of providing the
>>>> SCOPE_EXIT macros we could have said that the user writes the code to
>>>> bind the variables deducing their types and it programs the local
>>>> class with the exit code in the desstructor all of that by hand.
>>>> However, using macros saves the user to write all of that and as I
>>>> user I personally appreciate that. It is true that there is a trade
>>>> off between the readability cost of using macros and the benefit of
>>>> not having to write boiler-plate code. I find that trade off
>>>> acceptable for libraries like Boost.SopeExit or even ForEach and (of
>>>> course ;) ) also for my library so I don't have to write the
>>>> boiler-plate code for the binding and the casting to pass the local
>>>> class as a template parameter.
>>>
>>> Well, I didn't say that I think ScopeExit is good. In fact, I never saw
>>> the
>>
>> Can you please clarify? Would you make to ScopeExit the same
>> criticisms you are making to Boost.Local syntax and its macro usage or
>> not? // (1)
>
> Yes, I would make the criticism to ScopeExit.

OK, thanks a lot for clarifying your position on ScopeExit too.

>>> need to use it. As another reviewer noted, i'd rather encapsulate that in
>>> a
>>> class to logically divide that from the actual code.
>>> For ForEach the situation is completely different. Apart from having it
>>> to
>>> spell all caps, it looks exactly like a range based for each loop should
>>> look like. Boost.Local's Macros (yes, plural!) however, have nothing in
>>> common with the way you declare functions in C++.
>>
>> Again, I understand that you personally view the syntax as not
>> natural. (I am honestly sorry to hear that because I truly spent a lot
>> of effort in trying to make the syntax as natural as possible based on
>> everyone's input but I don't pretend to be able to make everyone happy
>> so it's ok.)
>
> Look, I am not trying to make you feel bad. I know that you put a lot of
> effort into making this library. If you remember, I wasn't happy about it
> from the start on, and i said so. As did others.
>
>>>>> The default parameter syntax is confusing, it looks like it is a
>>>>> completely
>>>>> new parameter, and not belonging to the previous argument. This gets
>>>>> confusing once you have more than one parameter with a default
>>>>> parameter.
>>>>> The WITH_DEFAULT macro should be the default, as it clearly states the
>>>>> relationship of the default value to the parameter.
>>>>
>>>> Others have made the same comments (with you a total of 3 people).
>>>> However, others have made the opposite comment (2 liked default, 5+
>>>> didn't say anything about it). In my experience, this quickly becomes
>>>> a non-issue after the 1st week that you start using the library.
>>>
>>> Right, i have to learn a new syntax in order to learn how to define local
>>
>> That is true for Boost.Lambda, Boost.Phenix, and even for C++11
>> (because most programmers are now only familiar with C++03). All these
>> tools require you to learn a new syntax as Boost.Local does. So what
>> is your point?
>
> The point is that the FP libraries offer additional value

As does my library. The additional value offered by my library is: (1)
functions can be defined locally, (2) statement syntax can be used to
program the function bodies, and (3) closures.

>>> functions. I note this in order to support the argument that Boost.Local
>>> does not solve the issues discussed above.
>>
>> Let me clarify that Boost.Local goal is to support local functions
>> with bindings and statement syntax for the function definition.
>> Boost.Local never tried to solve this problem without introducing a
>> new syntax for the function declaration (in fact earlier version of
>> the library had an even more exotic syntax for declaring the local
>> function).
>
> Right, looks like i just miss the point.
>
>>>>> Additionally as binding is probably the right term to use here, it
>>>>> might
>>>>> confuse people with the already existing solutions of the various bind
>>>>> implementations.
>>>>
>>>> I don't think so... here bind is clearly used in a different context.
>>>
>>> Yes that is what i meant. It is a overloading of terms.
>>
>> Why would it be confusing? You spent a few hours reading the docs, do
>> you honestly find it confusing? If so, do you think that a name
>> different than bind should be used? (I can use any name we can think
>> of and agree on.) For example, would "capture" be more clear:
>>
>> int x = 0;
>> void BOOST_LOCAL_FUNCTION_PARAMS(const capture&  x) {
>>     ... // some long set of instruction ;)
>> } BOOST_LOCAL_FUNCTION_NAME(f)
>> f();
>
> I never said it is confusing to me. I said to users. The comment above was
> trying to avoid overloading of terms. "bind" certainly has some history in
> boost and C++. That is all. You could argue that bind your context does
> something similar.

OK so let's use capture instead of bind.

>>>>> Local Blocks:
>>>>> I can't see the value in that one. The example in the documentation
>>>>> doesn't
>>>>> help either, as those things are easily detected by any modern
>>>>> compiler,
>>>>> with the appropriate warning levels (ok, not in the assert case, but
>>>>> for
>>>>> other boolean contexts at least gcc does).
>>>>
>>>> I am sure there are lots of people that will never use a local blocks.
>>>> That said, I personally needed to program (to ensure the
>>>> constant-correctness requirement of assertions in Contract
>>>> Programming):
>>>>
>>>> int x = 0;
>>>> const {
>>>>     assert( x == 0 );
>>>> }
>>>>
>>>> You can do that with local blocks:
>>>>
>>>> int x = 0;
>>>> BOOST_LOCAL_BLOCK(const bind x) {
>>>>     assert( x == 0 );
>>>> } BOOST_LOCAL_BLOCK_END
>>>>
>>>> As we said, using lambdas or other Boost libraries there are other
>>>> ways to do that but in my case I needed to keep the `assertion( x == 0
>>>> )`;` unchanged (including the variable name `x`) and these other ways
>>>> (including C++11 lambdas) don't allow for that.
>>>>
>>>> More in general, local blocks allow you to specify the sematic that a
>>>> variable should be thread as const by the next bunch of instruction. I
>>>> personally find that useful, others have made the same comment
>>>> (Vicente, Pierre, Jeff, and another couple of people-- see their
>>>> review to Boost.Local).
>>>
>>> Sorry, I just don't get it ... why does it need to be surrounded by a
>>> macro?
>>
>> Because I could not find any other way to declare the local function
>> using statement syntax for its definition without using macros. Does
>> this make sense?
>
> No. I was aiming at: Why do you need a local function at all, when all you
> want is a new scope where you don't have access to the outer variables? That
> sounds awfully familiar to what a normal function is.

All we want is (1) a function defined locally, (2) to use statement
syntax to program its body, and (3) to bind variables from the
enclosing environment. That is precisely what my library offers. It is
not a "normal" global function because (1) it not defined locally and
(3) it does not bind from the enclosing scope. It is not a "normal"
local function (member of a local class) because it does not bind
variables in scope (3) (plus it cannot be passed as template
parameter).

>>> I can't fully appreciate the value here. Not because of other existing
>>> libraries, but because normal functions are way more expressive and clear
>>> than these macros can ever be.
>>
>> But "normal" functions cannot be declared at local scope so they are
>> not sufficient for the problem domain of this library.
>
> This is exactly what I am trying to say. It looks to me that local is trying
> to solve a non-issue (at least for me).
> I argue that local functions are not needed in C++.

I don't understand... in your review you said:

On Fri, Nov 18, 2011 at 1:49 PM, Thomas Heller
<thom.heller_at_[hidden]> wrote:
>> - What is your evaluation of the potential usefulness of the library?
>
> The potential usefulness of the library is out of question, that is why
> C++11 now has lambdas, and libraries like Bind, Lambda and Phoenix exist.
> However, I do not believe that Local adds any value to the existing solution
> of the described problems.

So are you voting "no" because:
a) You don't think local functions are needed in C++.
b) Or, you don't like macros in general and you don't like Boost.Local
syntax in particular.
c) You do not believe that Boost.Local adds any value to existing
solutions to program local functions.

Thanks a lot for the clarifications.
--Lorenzo