[PATCH] Really *do* nothing in while loop

[PATCH] Really *do* nothing in while loop

[Thomas Glanzmann]

[PATCH] Really *do* nothing in while loop

Signed-Off-by: Thomas Glanzmann <sithglan@stud.uni-erlangen.de>

--- a/sha1_file.c
+++ b/sha1_file.c
@@ -335,7 +335,7 @@
 	stream.next_in = hdr;
 	stream.avail_in = hdrlen;
 	while (deflate(&stream, 0) == Z_OK)
-		/* nothing */
+		/* nothing */;
 
 	/* Then the data itself.. */
 	stream.next_in = buf;

Re: [PATCH] Really *do* nothing in while loop

[Michael Tokarev]

Thomas Glanzmann wrote:
> [PATCH] Really *do* nothing in while loop
> 
> Signed-Off-by: Thomas Glanzmann <sithglan@stud.uni-erlangen.de>
> 
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -335,7 +335,7 @@
>  	stream.next_in = hdr;
>  	stream.avail_in = hdrlen;
>  	while (deflate(&stream, 0) == Z_OK)
> -		/* nothing */
> +		/* nothing */;
>  
>  	/* Then the data itself.. */
>  	stream.next_in = buf;

Well, the lack of semicolon is wrong really (and funny).

But is the whole while loop needed at all?  deflate()
consumes as much input as it can, producing as much output
as it can.  So without the loop, and without updating the
buffer pointers ({next,avail}_{in,out}) it will do just
fine without the loop, and will return something != Z_OK
on next iteration.  If this is to mean to flush output,
it should be deflate(&stream, Z_FLUSH) or something.

/mjt

P.S.  What's git@vger.kernel.org for ?

Re: [PATCH] Really *do* nothing in while loop

[Thomas Glanzmann]

Hello,

> >-		/* nothing */
> >+		/* nothing */;

> Well, the lack of semicolon is wrong really (and funny).

yes, it is but harmless in this envrionment.

> But is the whole while loop needed at all?  deflate()
> consumes as much input as it can, producing as much output
> as it can.  So without the loop, and without updating the
> buffer pointers ({next,avail}_{in,out}) it will do just
> fine without the loop, and will return something != Z_OK
> on next iteration.  If this is to mean to flush output,
> it should be deflate(&stream, Z_FLUSH) or something.

I have no idea.

> P.S.  What's git@vger.kernel.org for ?

It is the list which handles GIT related discussions. Frontend/backend
and isn't kernel related.

	Thomas

Re: [PATCH] Really *do* nothing in while loop

[James Purser]

On Sun, 2005-05-08 at 19:48, Michael Tokarev wrote:
> Thomas Glanzmann wrote:
> > [PATCH] Really *do* nothing in while loop
> > 
> > Signed-Off-by: Thomas Glanzmann <sithglan@stud.uni-erlangen.de>
> > 
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -335,7 +335,7 @@
> >  	stream.next_in = hdr;
> >  	stream.avail_in = hdrlen;
> >  	while (deflate(&stream, 0) == Z_OK)
> > -		/* nothing */
> > +		/* nothing */;
> >  
> >  	/* Then the data itself.. */
> >  	stream.next_in = buf;
> 
> Well, the lack of semicolon is wrong really (and funny).
> 
> But is the whole while loop needed at all?  deflate()
> consumes as much input as it can, producing as much output
> as it can.  So without the loop, and without updating the
> buffer pointers ({next,avail}_{in,out}) it will do just
> fine without the loop, and will return something != Z_OK
> on next iteration.  If this is to mean to flush output,
> it should be deflate(&stream, Z_FLUSH) or something.
> 
> /mjt
> 
> P.S.  What's git@vger.kernel.org for ?
Its the mailing list for git development.
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
James Purser
http://ksit.dynalias.com

Re: [PATCH] Really *do* nothing in while loop

[jdow]

From: "James Purser" <purserj@ksit.dynalias.com>

> On Sun, 2005-05-08 at 19:48, Michael Tokarev wrote:
> > Thomas Glanzmann wrote:
> > > [PATCH] Really *do* nothing in while loop
> > > 
> > > Signed-Off-by: Thomas Glanzmann <sithglan@stud.uni-erlangen.de>
> > > 
> > > --- a/sha1_file.c
> > > +++ b/sha1_file.c
> > > @@ -335,7 +335,7 @@
> > >  stream.next_in = hdr;
> > >  stream.avail_in = hdrlen;
> > >  while (deflate(&stream, 0) == Z_OK)
> > > - /* nothing */
> > > + /* nothing */;
> > >  
> > >  /* Then the data itself.. */
> > >  stream.next_in = buf;
> > 
> > Well, the lack of semicolon is wrong really (and funny).

You guys REALLY do not see the changed semantics here? You are
changing:
  while (deflate(&stream, 0) == Z_OK)
      stream.next_in = buf;

into

  while (deflate(&stream, 0) == Z_OK)
    ;
  /* Then the data itself.. */
  stream.next_in = buf;

I suspect the results of that tiny bit of code would be slightly
different, especially if "stream.next_in" is volatile, "buf"
is volatile, or if the assignment to next_in has an effect on
the "deflate" operation.

{^_^}

Re: [PATCH] Really *do* nothing in while loop

[Michael Tokarev]

jdow wrote:
> From: "James Purser" <purserj@ksit.dynalias.com>
> 
   while (deflate(&stream, 0) == Z_OK)
-  /* nothing */
+  /* nothing */;
  stream.next_in = buf;
> 
> You guys REALLY do not see the changed semantics here? You are
> changing:
>   while (deflate(&stream, 0) == Z_OK)
>       stream.next_in = buf;
> 
> into
> 
>   while (deflate(&stream, 0) == Z_OK)
>     ;
>   /* Then the data itself.. */
>   stream.next_in = buf;
> 
> I suspect the results of that tiny bit of code would be slightly
> different, especially if "stream.next_in" is volatile, "buf"
> is volatile, or if the assignment to next_in has an effect on
> the "deflate" operation.

As I already said, deflate() in this case does only ONE iteration.
stream.avail_in is NOT changed in the loop (except of the deflate()
itself, where it will be set to 0 - provided out buffer have enouth
room).  So the whole while loop does only ONE iteration, returning
Z_NEED_DATA or something the next one.  So no, the semantics here
(actual semantics) does NOT change.

/mjt

Re: [PATCH] Really *do* nothing in while loop [OT - style]

[Eyal Lebedinsky]

Thomas Glanzmann wrote:
> [PATCH] Really *do* nothing in while loop
> 
> Signed-Off-by: Thomas Glanzmann <sithglan@stud.uni-erlangen.de>
> 
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -335,7 +335,7 @@
>  	stream.next_in = hdr;
>  	stream.avail_in = hdrlen;
>  	while (deflate(&stream, 0) == Z_OK)
> -		/* nothing */
> +		/* nothing */;

I am explicitly ignoring the core subject on this thread. This
is a side note, regarding coding style.

I always use a common format for an empty body:

	while (deflate(&stream, 0) == Z_OK)
		{}

It stands out and positively says what is meant. As such it
makes it much more readable.

I think that CodingStyle should provide guidance for empty
bodies.

-- 
Eyal Lebedinsky (eyal@eyal.emu.id.au) <http://samba.org/eyal/>
	attach .zip as .dat

Re: [PATCH] Really *do* nothing in while loop

[Junio C Hamano]

>>>>> "MT" == Michael Tokarev <mjt@tls.msk.ru> writes:

MT> As I already said, deflate() in this case does only ONE iteration.
MT> stream.avail_in is NOT changed in the loop (except of the deflate()
MT> itself, where it will be set to 0 - provided out buffer have enouth
MT> room)....

Just a stupid question, but what happens when we do not have
enough room in the buffer?

Re: [PATCH] Really *do* nothing in while loop

[Daniel Barkalow]

On Sun, 8 May 2005, Junio C Hamano wrote:

> >>>>> "MT" == Michael Tokarev <mjt@tls.msk.ru> writes:
> 
> MT> As I already said, deflate() in this case does only ONE iteration.
> MT> stream.avail_in is NOT changed in the loop (except of the deflate()
> MT> itself, where it will be set to 0 - provided out buffer have enouth
> MT> room)....
> 
> Just a stupid question, but what happens when we do not have
> enough room in the buffer?

We must have enough room; we sized the buffer with deflateBound to fit the
worst case.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Really *do* nothing in while loop

[Junio C Hamano]

>>>>> "DB" == Daniel Barkalow <barkalow@iabervon.org> writes:

DB> On Sun, 8 May 2005, Junio C Hamano wrote:
>> 
MT> As I already said, deflate() in this case does only ONE iteration.
MT> stream.avail_in is NOT changed in the loop (except of the deflate()
MT> itself, where it will be set to 0 - provided out buffer have enouth
MT> room)....
>> 
>> Just a stupid question, but what happens when we do not have
>> enough room in the buffer?

DB> We must have enough room; we sized the buffer with deflateBound to fit the
DB> worst case.

Thanks for the explanation.  Then do we still need to express it
as a while loop?

Re: [PATCH] Really *do* nothing in while loop

[J.A. Magallon]

On 05.08, Michael Tokarev wrote:
> Thomas Glanzmann wrote:
> > [PATCH] Really *do* nothing in while loop
> > 
> > Signed-Off-by: Thomas Glanzmann <sithglan@stud.uni-erlangen.de>
> > 
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -335,7 +335,7 @@
> >  	stream.next_in = hdr;
> >  	stream.avail_in = hdrlen;
> >  	while (deflate(&stream, 0) == Z_OK)
> > -		/* nothing */
> > +		/* nothing */;
> >  
> >  	/* Then the data itself.. */
> >  	stream.next_in = buf;
> 
> Well, the lack of semicolon is wrong really (and funny).
> 
> But is the whole while loop needed at all?  deflate()
> consumes as much input as it can, producing as much output
> as it can.  So without the loop, and without updating the
> buffer pointers ({next,avail}_{in,out}) it will do just
> fine without the loop, and will return something != Z_OK
> on next iteration.  If this is to mean to flush output,
> it should be deflate(&stream, Z_FLUSH) or something.
> 

This changes the code in the corner case when deflate(...) IS NOT Z_OK
in the first iteration.
Old code: next_in is not assigned if deflate(&stream, 0) != Z_OK
New code: next_is is _always_ assigned

Other point is if old code was wrong...hidden bug ?

--
J.A. Magallon <jamagallon()able!es>     \               Software is like sex:
werewolf!able!es                         \         It's better when it's free
Mandriva Linux release 2006.0 (Cooker) for i586
Linux 2.6.11-jam16 (gcc 4.0.0 (4.0.0-3mdk for Mandriva Linux release 2006.0))