[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