Re: [boost] [local] Review request

Subject: Re: [boost] [local] Review request
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2011-05-15 14:47:12

On Sun, May 15, 2011 at 12:42 AM, Mostafa
<mostafa_working_away_at_[hidden]> wrote:
> On Sat, 14 May 2011 18:24:36 -0700, Lorenzo Caminiti <lorcaminiti_at_[hidden]>
> wrote:
>
>> On Sat, May 14, 2011 at 9:06 PM, Mostafa <mostafa_working_away_at_[hidden]>
>> wrote:
>>>
>>> On Sat, 14 May 2011 12:38:50 -0700, Lorenzo Caminiti
>>> <lorcaminiti_at_[hidden]>
>>> I just took a quick glance at the documentation to get an understanding
>>> of
>>> the library, and I have a suggestion/comment:
>>>
>>> 1) I suggest adding:
>>>
>>> #ifdef ENABLE_BOOST_LOCAL_VARIADIC_WITH_DEFAULT
>>> #define WITH_DEFAULT , default
>>> #endif
>>>
>>> #ifdef ENABLE_BOOST_LOCAL_SEQUENCING_WITH_DEFAULT
>>> #define WITH_DEFAULT ) default
>>> #endif
>>>
>>> to the library.  I think it makes client code more readable if they
>>> define
>>> ENABLE_BOOST_LOCAL_VARIADIC_WITH_DEFAULT or its variant rather than just
>>> defining WITH_DEFAULT.
>>
>> Something similar (at least for the variadic syntax) is suggest in one
>> of the docs examples -- see last example here:
>>
>> http://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_local/Tutorial.html#boost_local.Tutorial.default_parameters
>>
>> However, Boost macro naming conventions will require this macro to be
>> named BOOST_LOCAL_WITH_DEFAULT if the macro were to be added to
>> Boost.Local. IMO, that name is too long defeating the increased
>> readability benefit. Therefore, I'd leave it up to programmers to
>> #define WITH_DEFAULT if they wish to do so as suggested by the above
>> doc example.
>
> Yes, it was from the documentation where I originally got my motivation
> from.  Just to be clear, I'm not suggesting replacing WITH_DEFAULT with
> BOOST_LOCAL_WITH_DEFAULT, or with my equivalents.  Rather I was suggesting a
> "standard" switch be available which, if defined by the client, would enable
> the WITH_DEFAULT macro.  As to the name being too long, I envisioned it
> (BOOST_LOCAL_WITH_DEFAULT or its equivalents) to be defined only *once* by
> the clients in a common header file, and then they would use the
> subsequently available WITH_DEFAULT through out the rest of their code.
>
> I guess I'm just being super lazy, because if I were going to define
> WITH_DEFAULT myself, then I would have to explicitly add a comment that its
> for use with Boost.Local, else it would look like a very strangely defined
> macro that would require a grep to figure out what it's used for.  (I like
> self documenting code, hence the suggested names.)

Boost will require this macro to be named BOOST_LOCAL_WITH_DEFAULT
even if it is controlled by the
BOOST_LOCAL_ENABLE_BOOST_LOCAL_[VARIADIC/SEQUENCING]_WITH_DEFAULT
switch:
http://www.boost.org/development/requirements.html#Design_and_Programming

I found this macro names to be longer and less readable than ",
default x" or ")(default x" so I decided not to add
BOOST_LOCAL_WITH_DEFAULT to the library defined macros. I simply to
suggest in the library docs that programmers can define this macro if
they find it readable. If during the review also other programmers
request to add BOOST_LOCAL_WITH_DEFAULT, I am happy to consider it.

--Lorenzo