Date: Thu, 13 Aug 2015 12:16:41 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Simpson <bms@fastmail.net> Cc: Baptiste Daroussin <bapt@freebsd.org>, Davide Italiano <davide@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, svn-src-projects@freebsd.org Subject: Re: svn commit: r286484 - projects/collation/usr.bin/localedef Message-ID: <20150813114425.X996@besplex.bde.org> In-Reply-To: <55CB91FD.8000004@fastmail.net> References: <201508082257.t78MvIT1000841@repo.freebsd.org> <CACYV=-GnOpPddd-x_J0yW4g4QFsdcEVXaVc9CER9JD7iObGzAg@mail.gmail.com> <20150812182739.GB51754@ivaldir.etoilebsd.net> <55CB91FD.8000004@fastmail.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 12 Aug 2015, Bruce Simpson wrote:
> On 12/08/15 19:27, Baptiste Daroussin wrote:
>> On Wed, Aug 12, 2015 at 01:22:17PM -0400, Davide Italiano wrote:
>>>> +#define RB_NUMNODES(type, name, head, cnt) do { \
>>>> + type *t; \
>>>> + cnt = 0; \
>>>> + RB_FOREACH(t, name, head) { \
>>>> + cnt++; \
>>>> + } \
>>>> +} while (0);
>>>> +
>>>
>>> Can you commit this one to HEAD && move it to the right header?
This has too many bugs to commit.
>> You mean adding to tree(3)?
>
> Not sure why you'd want to pollute it by doing this. The macro is simple
> enough that anyone can write it, and it is often best to count RB nodes
> whilst doing something else (or lazy-update) to avoid unnecessary traversals.
It is apparently not that simple. The above has the following bugs:
- gross: semicolon after while(0) defeats the reason for existence of the
do-while(0) hack
- all macro args are used without parentheses
- the variable 't' is in the application namespace
- minor style bugs:
- no blank line after declarations
- backslashes not line up
sys/tree.h has a lower density of such bugs. The most obvious ones are:
- SPLAY_LEFT/RIGHT/ROOT() not parenthesized
- 'tmp' arg not parenthesized in most or all macros
- bogus parenthesization of 'head' in some places in function definition
macros. It must be an identifier to work there, unlike in other macros
- bogus double underscores in names in function definitions. Some
underscores are needed to avoid the above bug for 't'. Single ones
are sufficient and minimise the uglyness. That is in functions. In
macros, but probably only outside of inner blocks like the one created
by the do-while(0) hack, double underscores might be needed.
- -1 is not parenthesized
- the RB macros are of even lower quality. They have simiilar bugs, plus
they use no underscores for most local variables where the SPLAY macros
carefully use excessive underscores.
- there are some style bugs, but their size and density is lower than the
technical bugs.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150813114425.X996>