[RFC] CodingStyle + [PATCH] to wget.c

Denis Vlasenko vda at ilport.com.ua
Mon Feb 27 08:09:15 UTC 2006


On Friday 24 February 2006 18:18, Rob Landley wrote:
> > But really, conditionals can be complex: x && (y || z) and no style
> > can sanely fold it into nice readable structure.
> > It's best to avoid placing such monstrosities in if's.
> >
> > See attached patch to wget.c which fixes monster
> > while(..............................................)
> > as an example.
> 
> A) Did you compare make sizes on the two of them?
> 
> B) 
> 
> +			off_t size = sizeof(buf);
> +			if (chunked || got_clen)
> +				if (filesize < sizeof(buf))
> +					size = filesize;
> 
> So you copy sizeof() into a variable, and then call it again rather than using 
> the value in the variable?

The extended answer:

wget_org - original code
wget_new - after my patch
wget_new2 - with "if (filesize < sizeof(buf))" -> "if (filesize < size)"

# size wget*.o
   text    data     bss     dec     hex filename
   7161       0      60    7221    1c35 wget_new.o
   7162       0      60    7222    1c36 wget_new2.o
   7161       0      60    7221    1c35 wget_org.o

# diff -u wget_new.s wget_new2.s
--- wget_new.s  Mon Feb 27 09:52:42 2006
+++ wget_new2.s Mon Feb 27 09:55:43 2006
@@ -1898,11 +1898,11 @@
        cmpl    $0, -1776(%ebp)
        je      .L254
 .L255:
-       testl   %ecx, %ecx
+       cmpl    $0, %ecx
        jg      .L254
        jl      .L257
-       cmpl    $511, %edx
-       ja      .L254
+       cmpl    $512, %edx
+       jae     .L254
 .L257:
        movl    %edx, %ebx
 .L254:

As you see here, gcc generated basically tha same code for new and new2.
I have no idea why compiler uses "cmpl $0, %ecx". But it does.

In other words: in this particular case, prettifying
that while() costs nothing.

Frankly, I suspect than different version of gcc
can use a few more (or less) bytes. Even if it will,
I think far more readable code worth extra byte or two.
We have plenty of places where we can shave off _kilobytes_.

Gory details:

# diff -u wget_org.s wget_new.s
--- wget_org.s	Mon Feb 27 09:53:14 2006
+++ wget_new.s	Mon Feb 27 09:52:42 2006
@@ -1890,34 +1890,22 @@ wget_main:
 #APP
 	#1
 #NO_APP
-.L286:
-	movl	filesize+4, %edx
-	testl	%edx, %edx
-	movl	filesize, %eax
-	jg	.L252
-	jl	.L253
-	cmpl	$0, %eax
-	ja	.L252
-.L253:
-	cmpl	$0, -1776(%ebp)
-	jne	.L251
-.L252:
+	jmp	.L288
+.L269:
 	cmpl	$0, chunked
-	jne	.L256
+	movl	$512, %ebx
+	jne	.L255
 	cmpl	$0, -1776(%ebp)
 	je	.L254
-.L256:
-	testl	%edx, %edx
+.L255:
+	testl	%ecx, %ecx
 	jg	.L254
 	jl	.L257
-	cmpl	$511, %eax
+	cmpl	$511, %edx
 	ja	.L254
 .L257:
-	movl	filesize, %ebx
-	jmp	.L255
+	movl	%edx, %ebx
 .L254:
-	movl	$512, %ebx
-.L255:
 	xorl	%esi, %esi
 .L258:
 	pushl	%edi
@@ -1967,34 +1955,44 @@ wget_main:
 	addl	%eax, %ebx
 	addl	$16, %esp
 	cmpl	%esi, %ebx
-	jae	.L264
+	jae	.L265
 	pushl	-1780(%ebp)
 	call	ferror
 	testl	%eax, %eax
 	popl	%edx
-	je	.L264
+	je	.L265
 	call	__errno_location
 	cmpl	$4, (%eax)
-	je	.L263
-.L264:
+	je	.L264
+.L265:
 	cmpl	-1744(%ebp), %ebx
-	je	.L262
+	je	.L263
 	pushl	$.LC84
-	jmp	.L282
-.L262:
+	jmp	.L284
+.L263:
 	movl	statbytes, %eax
 	addl	%ebx, %eax
 	cmpl	$0, -1776(%ebp)
 	movl	%eax, statbytes
-	je	.L286
+	je	.L288
 	movl	%ebx, %eax
 	cltd
 	subl	%ebx, filesize
 	sbbl	%edx, filesize+4
-	jmp	.L286
+.L288:
+	movl	filesize+4, %ecx
+	testl	%ecx, %ecx
+	movl	filesize, %edx
+	jg	.L269
+	jl	.L253
+	cmpl	$0, %edx
+	ja	.L269
+.L253:
+	cmpl	$0, -1776(%ebp)
+	je	.L269
 .L251:
 	cmpl	$0, chunked
-	je	.L268
+	je	.L270
 	leal	-524(%ebp), %ebx
 	pushl	%edi
 	movl	$512, %edx
--
vda



More information about the busybox mailing list