From owner-svn-src-projects@freebsd.org Thu Aug 13 02:16:54 2015 Return-Path: Delivered-To: svn-src-projects@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 1A25E99F88C for ; Thu, 13 Aug 2015 02:16:54 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id 8B22496D; Thu, 13 Aug 2015 02:16:53 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail107.syd.optusnet.com.au (Postfix) with ESMTPS id 653D8D483AB; Thu, 13 Aug 2015 12:16:42 +1000 (AEST) Date: Thu, 13 Aug 2015 12:16:41 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Simpson cc: Baptiste Daroussin , Davide Italiano , "src-committers@freebsd.org" , svn-src-projects@freebsd.org Subject: Re: svn commit: r286484 - projects/collation/usr.bin/localedef In-Reply-To: <55CB91FD.8000004@fastmail.net> Message-ID: <20150813114425.X996@besplex.bde.org> References: <201508082257.t78MvIT1000841@repo.freebsd.org> <20150812182739.GB51754@ivaldir.etoilebsd.net> <55CB91FD.8000004@fastmail.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=ItbjC+Lg c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=kj9zAlcOel0A:10 a=VCLdKcpBrK-A4TL3LXUA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 13 Aug 2015 02:16:54 -0000 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