Re: PATCH: merge.c white space cleanup
On Mon, Jan 28, 2013 at 08:45:19PM +0100, Marc Espie wrote: > > > Otoh this file is quite bad. It makes my vim light up all red. > > > > That's why you should stick to vi and not the emacs-in-a-vi-disguise > > bloatware known as vim. > > Lol. > > Give me a vi with multiple windows, and with visual, and I will consider > changing editors. "vi with visual" sounds like a tautology to me ;-) Ciao, Kili
Re: PATCH: merge.c white space cleanup
> Give me a vi with multiple windows, and with visual, and I will consider
> changing editors.
If you need multiple windows, simply open several {rxvt,xterm,whatever}
windows and run vi in them.
Re: PATCH: merge.c white space cleanup
On Mon, Jan 28, 2013 at 07:03:24PM +, Miod Vallat wrote: > > Usually whitespace diffs are not that well liked, they make for > > annoying differences, introduce noise and require more effort to review > > for questionable gain. > > > > Otoh this file is quite bad. It makes my vim light up all red. > > That's why you should stick to vi and not the emacs-in-a-vi-disguise > bloatware known as vim. Lol. Give me a vi with multiple windows, and with visual, and I will consider changing editors.
Re: PATCH: merge.c white space cleanup
> Usually whitespace diffs are not that well liked, they make for
> annoying differences, introduce noise and require more effort to review
> for questionable gain.
>
> Otoh this file is quite bad. It makes my vim light up all red.
That's why you should stick to vi and not the emacs-in-a-vi-disguise
bloatware known as vim.
> I'm still torn on this. Anyone else care to comment?
If this file is to be changed, then PLEASE fix the logic to get rid of
the gotos.
I did this more than 10 years ago (see diff below), but never dared to
expose it to enough real-word testcases to be confident in this.
Reviewers welcome. Be warned it might not apply cleanly anymore...
Index: merge.c
===
RCS file: /cvs/src/lib/libc/stdlib/merge.c,v
retrieving revision 1.5
diff -u -r1.5 merge.c
--- merge.c 2002/02/17 19:42:24 1.5
+++ merge.c 2002/12/19 02:12:41
@@ -1,3 +1,4 @@
+/* $OpenBSD$ */
/*-
* Copyright (c) 1992, 1993
* The Regents of the University of California. All rights reserved.
@@ -52,7 +53,7 @@
* (The default is pairwise merging.)
*/
-#include
+#include
#include
#include
@@ -63,33 +64,30 @@
#define ISIZE sizeof(int)
#define PSIZE sizeof(u_char *)
-#define ICOPY_LIST(src, dst, last) \
- do \
- *(int*)dst = *(int*)src, src += ISIZE, dst += ISIZE;\
- while(src < last)
-#define ICOPY_ELT(src, dst, i) \
- do \
- *(int*) dst = *(int*) src, src += ISIZE, dst += ISIZE; \
- while (i -= ISIZE)
+#define ICOPY_LIST(src, dst, last) \
+ do {\
+ *(int*)dst = *(int*)src, src += ISIZE, dst += ISIZE;\
+ } while(src < last)
+#define ICOPY_ELT(src, dst, i) \
+ do {\
+ *(int*) dst = *(int*) src, src += ISIZE, dst += ISIZE; \
+ } while (i -= ISIZE)
#define CCOPY_LIST(src, dst, last) \
- do \
+ do {\
*dst++ = *src++;\
- while (src < last)
+ } while (src < last)
#define CCOPY_ELT(src, dst, i) \
- do \
+ for (; i; i--) {\
*dst++ = *src++;\
- while (i -= 1)
+ }
/*
* Find the next possible pointer head. (Trickery for forcing an array
* to do double duty as a linked list when objects do not align with word
* boundaries.
*/
-/* Assumption: PSIZE is a power of 2. */
-#define EVAL(p) (u_char **)\
- ((u_char *)0 + \
- (((u_char *)p + PSIZE - 1 - (u_char *) 0) & ~(PSIZE - 1)))
+#define EVAL(p)(u_char **)roundup((long)p, PSIZE)
/*
* Arguments are as for qsort.
@@ -165,7 +163,14 @@
t = p;
if (i == size)
big = 0;
- goto FASTCASE;
+
+ while (i > size)
+ if ((*cmp)(q,
+ p = b + (i >>= 1))
+ <= sense)
+ t = p;
+ else
+ b = p;
} else
b = p;
while (t > b+size) {
@@ -175,14 +180,7 @@
else
b = p;
}
- goto COPY;
-FASTCASE: while (i > size)
- if ((*cmp)(q,
- p = b + (i >>= 1)) <= sense)
- t = p;
- else
- b = p;
-COPY: b = t;
+ b = t;
}
i = size;
if (q == f1) {
@@ -231,21 +229,19 @@
#defineswap(a, b) {\
s = b;
Re: PATCH: merge.c white space cleanup
On Thu, Jan 24, 2013 at 09:37:33PM +0200, Ville Valkonen wrote:
> Hi,
>
> save a few precious bytes by removing unnecessary white spaces from
> libc/merge.c.
Please send diffs inline, it's a lot easier to review.
Usually whitespace diffs are not that well liked, they make for
annoying differences, introduce noise and require more effort to review
for questionable gain.
Otoh this file is quite bad. It makes my vim light up all red. The diff
itself is correct, the whitespace changes do not introduce any hidden
code changes as per sha1. From that perspective it's got my ok.
I'm still torn on this. Anyone else care to comment?
Same diff, as tested:
Index: stdlib/merge.c
===
RCS file: /home/vcs/cvs/openbsd/src/lib/libc/stdlib/merge.c,v
retrieving revision 1.9
diff -u -p -r1.9 merge.c
--- stdlib/merge.c 6 Mar 2011 00:55:38 - 1.9
+++ stdlib/merge.c 27 Jan 2013 18:29:30 -
@@ -73,7 +73,7 @@ static void insertionsort(u_char *, size
do \
*dst++ = *src++;\
while (i -= 1)
-
+
/*
* Find the next possible pointer head. (Trickery for forcing an array
* to do double duty as a linked list when objects do not align with word
@@ -120,91 +120,91 @@ mergesort(void *base, size_t nmemb, size
l2 = list1;
p1 = EVAL(list1);
for (tp2 = p2 = list2; p2 != last; p1 = EVAL(l2)) {
- p2 = *EVAL(p2);
- f1 = l2;
- f2 = l1 = list1 + (p2 - list2);
- if (p2 != last)
- p2 = *EVAL(p2);
- l2 = list1 + (p2 - list2);
- while (f1 < l1 && f2 < l2) {
- if ((*cmp)(f1, f2) <= 0) {
- q = f2;
- b = f1, t = l1;
- sense = -1;
- } else {
- q = f1;
- b = f2, t = l2;
- sense = 0;
- }
- if (!big) { /* here i = 0 */
- while ((b += size) < t && cmp(q, b) >sense)
- if (++i == 6) {
- big = 1;
- goto EXPONENTIAL;
- }
- } else {
-EXPONENTIAL: for (i = size; ; i <<= 1)
- if ((p = (b + i)) >= t) {
- if ((p = t - size) > b &&
+ p2 = *EVAL(p2);
+ f1 = l2;
+ f2 = l1 = list1 + (p2 - list2);
+ if (p2 != last)
+ p2 = *EVAL(p2);
+ l2 = list1 + (p2 - list2);
+ while (f1 < l1 && f2 < l2) {
+ if ((*cmp)(f1, f2) <= 0) {
+ q = f2;
+ b = f1, t = l1;
+ sense = -1;
+ } else {
+ q = f1;
+ b = f2, t = l2;
+ sense = 0;
+ }
+ if (!big) { /* here i = 0 */
+ while ((b += size) < t && cmp(q, b) >sense)
+ if (++i == 6) {
+ big = 1;
+ goto EXPONENTIAL;
+ }
+ } else {
+EXPONENTIAL: for (i = size; ; i <<= 1)
+ if ((p = (b + i)) >= t) {
+ if ((p = t - size) > b &&
(*cmp)(q, p) <= sense)
- t = p;
- else
- b = p;
- break;
- } else if ((*cmp)(q, p) <= sense) {
- t = p;
- if (i == size)
- big = 0;
- goto FASTCASE;
- } else
- b = p;
- while (t > b+size) {
- i = (((t - b) / size) >> 1) * size;
- if ((*cmp)(q, p = b + i) <= sense)
- t = p;
- else
-
PATCH: merge.c white space cleanup
Hi, save a few precious bytes by removing unnecessary white spaces from libc/merge.c. http://weezel.fsck.fi/merge.patch> -- Sincerely, Ville Valkonen
