On Sun, May 15, 2011 at 12:42 AM, Mostafa <mostafa_working_away@yahoo.com> wrote:
On Sat, 14 May 2011 18:24:36 -0700, Lorenzo Caminiti <lorcaminiti@gmail.com> wrote:
On Sat, May 14, 2011 at 9:06 PM, Mostafa <mostafa_working_away@yahoo.com> wrote:
On Sat, 14 May 2011 12:38:50 -0700, Lorenzo Caminiti <lorcaminiti@gmail.com> 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...
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