Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 3 Apr 2014 18:51:43 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        d@delphij.net
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Xin LI <delphij@freebsd.org>
Subject:   Re: svn commit: r264059 - head/bin/dd
Message-ID:  <20140403183237.I1546@besplex.bde.org>
In-Reply-To: <533D0307.6090509@delphij.net>
References:  <201404030055.s330tGCI070764@svn.freebsd.org> <20140403150420.H959@besplex.bde.org> <533D0307.6090509@delphij.net>

next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 2 Apr 2014, Xin Li wrote:

> On 4/2/14, 9:45 PM, Bruce Evans wrote:
>> On Thu, 3 Apr 2014, Xin LI wrote:
>>> 0x00020 -#define    C_FILES        0x00040 -#define    C_IBS
>>> 0x00080 -#define    C_IF        0x00100 -#define    C_LCASE
>>> 0x00200 -#define    C_NOERROR    0x00400 -#define    C_NOTRUNC
>>> 0x00800 -#define    C_OBS        0x01000 -#define    C_OF
>>> 0x02000 -#define    C_OSYNC        0x04000 -#define    C_PAREVEN
>>> 0x08000 -#define    C_PARNONE    0x100000
>>
>> The previous formatting was not too good, and obfuscated a bug
>> here. 4 bits are unused.  Someone added this bug together with many
>> others in 2004, and I missed it then when I committed a fix for the
>> others. Most of the bugs that I fixed were unsorting of the table
>> by adding to its end.  Insertion sort tends to give large churn of
>> the table by changing all the numbers, but for the 2004 fix there
>> wasn't much churn except relative to the unsorted version because
>> all the changes were near the end.

Actually it was me who added the unused bits.  *Blush*.

I see you committed a fix.

>>> -#define    C_PARODD    0x200000 -#define    C_PARSET
>>> 0x400000 -#define    C_SEEK        0x800000 -#define    C_SKIP
>>> 0x1000000 -#define    C_SPARSE    0x2000000 -#define    C_SWAB
>>> 0x4000000 -#define    C_SYNC        0x8000000 -#define    C_UCASE

Mail programs keep getting worse.  Here they destroyed the line wrapping
together with rearranging the quotes.

> Will it be an improvement if we have the attached change committed?
> Basically it redo the sort and use 1 <<'s instead of hardcoded heximal
> numbers.

Too much style changing.  dd still uses hex constants for smaller flag
values elsewhere, even in the same file.

>> The 2 values above UINT_MAX also won't compile with compilers that
>> warn that 'integer constant is too large for "long" type'.  Mainly
>> gcc
> without
>> -std=c99.  clang is too incompatible to warn about this even with
>> -std=c89 -Wall.  The fix is not to further churn the table by
>> adding a ULL suffix to all entries.

Using shifts gives a similar type problem for 32-bit types.  From the
patch:

% -#define	C_NOXFER	0x10000000
% -#define	C_NOINFO	0x20000000
% ...
% +#define	C_UCASE		(1 << 28)
% +#define	C_UNBLOCK	(1 << 29)

This works up to (1 << 30), assuming 32-bit ints, but (1 << 31) would
overflow.  It would have to be written as (1U << 31) (keep assuming
32-bit ints), but this would mess up the fancy formatting.  Elswhere
in the patch:

% -#define	C_ASCII		0x00000001
% -#define	C_BLOCK		0x00000002
% ...
% +#define	C_ASCII		(1 <<  0)
% +#define	C_BLOCK		(1 <<  1)

Fancy formatting like this (the extra space before the shift count to line
things up) is hard to maintain since it is too fancy for indent(1) to produce
or preserve.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140403183237.I1546>