[Spice-devel,v2,01/11] server/red_worker: fix dump_bitmap

Submitted by Yonit Halperin on April 17, 2012, 10:12 a.m.

Details

Message ID 1334657556-5083-1-git-send-email-yhalperi@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Yonit Halperin April 17, 2012, 10:12 a.m.
---
 server/red_worker.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/red_worker.c b/server/red_worker.c
index 07782c8..5350195 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -11348,10 +11348,11 @@  static void dump_bitmap(RedWorker *worker, SpiceBitmap *bitmap, uint32_t group_i
     }
     /* writing the data */
     for (i = 0; i < bitmap->data->num_chunks; i++) {
+        int j;
         SpiceChunk *chunk = &bitmap->data->chunk[i];
         int num_lines = chunk->len / bitmap->stride;
-        for (i = 0; i < num_lines; i++) {
-            dump_line(f, chunk->data + (i * bitmap->stride), n_pixel_bits, bitmap->x, row_size);
+        for (j = 0; j < num_lines; j++) {
+            dump_line(f, chunk->data + (j * bitmap->stride), n_pixel_bits, bitmap->x, row_size);
         }
     }
     fclose(f);

Comments

On Tue, Apr 17, 2012 at 01:12:26PM +0300, Yonit Halperin wrote:
> ---
>  server/red_worker.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 07782c8..5350195 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -11348,10 +11348,11 @@ static void dump_bitmap(RedWorker *worker, SpiceBitmap *bitmap, uint32_t group_i
>      }
>      /* writing the data */
>      for (i = 0; i < bitmap->data->num_chunks; i++) {
> +        int j;

Missed this in v1, sorry - shouldn't this be defined at the function
start? Not a big deal.

>          SpiceChunk *chunk = &bitmap->data->chunk[i];
>          int num_lines = chunk->len / bitmap->stride;
> -        for (i = 0; i < num_lines; i++) {
> -            dump_line(f, chunk->data + (i * bitmap->stride), n_pixel_bits, bitmap->x, row_size);
> +        for (j = 0; j < num_lines; j++) {
> +            dump_line(f, chunk->data + (j * bitmap->stride), n_pixel_bits, bitmap->x, row_size);
>          }
>      }
>      fclose(f);
> -- 
> 1.7.7.6
>
On 04/17/2012 01:46 PM, Alon Levy wrote:
> On Tue, Apr 17, 2012 at 01:12:26PM +0300, Yonit Halperin wrote:
>> ---
>>   server/red_worker.c |    5 +++--
>>   1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/server/red_worker.c b/server/red_worker.c
>> index 07782c8..5350195 100644
>> --- a/server/red_worker.c
>> +++ b/server/red_worker.c
>> @@ -11348,10 +11348,11 @@ static void dump_bitmap(RedWorker *worker, SpiceBitmap *bitmap, uint32_t group_i
>>       }
>>       /* writing the data */
>>       for (i = 0; i<  bitmap->data->num_chunks; i++) {
>> +        int j;
>
> Missed this in v1, sorry - shouldn't this be defined at the function
> start? Not a big deal.
why?
>
>>           SpiceChunk *chunk =&bitmap->data->chunk[i];
>>           int num_lines = chunk->len / bitmap->stride;
>> -        for (i = 0; i<  num_lines; i++) {
>> -            dump_line(f, chunk->data + (i * bitmap->stride), n_pixel_bits, bitmap->x, row_size);
>> +        for (j = 0; j<  num_lines; j++) {
>> +            dump_line(f, chunk->data + (j * bitmap->stride), n_pixel_bits, bitmap->x, row_size);
>>           }
>>       }
>>       fclose(f);
>> --
>> 1.7.7.6
>>
On Tue, Apr 17, 2012 at 01:46:26PM +0300, Alon Levy wrote:
> On Tue, Apr 17, 2012 at 01:12:26PM +0300, Yonit Halperin wrote:
> > ---
> >  server/red_worker.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 07782c8..5350195 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -11348,10 +11348,11 @@ static void dump_bitmap(RedWorker *worker, SpiceBitmap *bitmap, uint32_t group_i
> >      }
> >      /* writing the data */
> >      for (i = 0; i < bitmap->data->num_chunks; i++) {
> > +        int j;
> 
> Missed this in v1, sorry - shouldn't this be defined at the function
> start? Not a big deal.

In my opinion, it's better to keep variable declaration in the smallest
enclosing block.

Christophe
On Tue, Apr 17, 2012 at 01:37:26PM +0200, Christophe Fergeau wrote:
> On Tue, Apr 17, 2012 at 01:46:26PM +0300, Alon Levy wrote:
> > On Tue, Apr 17, 2012 at 01:12:26PM +0300, Yonit Halperin wrote:
> > > ---
> > >  server/red_worker.c |    5 +++--
> > >  1 files changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 07782c8..5350195 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -11348,10 +11348,11 @@ static void dump_bitmap(RedWorker *worker, SpiceBitmap *bitmap, uint32_t group_i
> > >      }
> > >      /* writing the data */
> > >      for (i = 0; i < bitmap->data->num_chunks; i++) {
> > > +        int j;
> > 
> > Missed this in v1, sorry - shouldn't this be defined at the function
> > start? Not a big deal.
> 
> In my opinion, it's better to keep variable declaration in the smallest
> enclosing block.

I agree it's better. I commented out of a misplaced thought we should
support C-something compilers that don't allow it. I take it back.

> 
> Christophe



> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Tue, Apr 17, 2012 at 02:51:33PM +0300, Alon Levy wrote:
> On Tue, Apr 17, 2012 at 01:37:26PM +0200, Christophe Fergeau wrote:
> > On Tue, Apr 17, 2012 at 01:46:26PM +0300, Alon Levy wrote:
> > > On Tue, Apr 17, 2012 at 01:12:26PM +0300, Yonit Halperin wrote:
> > > > ---
> > > >  server/red_worker.c |    5 +++--
> > > >  1 files changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > > index 07782c8..5350195 100644
> > > > --- a/server/red_worker.c
> > > > +++ b/server/red_worker.c
> > > > @@ -11348,10 +11348,11 @@ static void dump_bitmap(RedWorker *worker, SpiceBitmap *bitmap, uint32_t group_i
> > > >      }
> > > >      /* writing the data */
> > > >      for (i = 0; i < bitmap->data->num_chunks; i++) {
> > > > +        int j;
> > > 
> > > Missed this in v1, sorry - shouldn't this be defined at the function
> > > start? Not a big deal.
> > 
> > In my opinion, it's better to keep variable declaration in the smallest
> > enclosing block.
> 
> I agree it's better. I commented out of a misplaced thought we should
> support C-something compilers that don't allow it. I take it back.

Oh, declaring variables at the beginning of a C block as is done here is
supported by all C compilers I know of. It's declaring variables in the
middle of a block (after some non-declarations lines of code) which is not
supported by all compilers. In this case, I don't think this will cause any
issues.

Christophe
On Tue, Apr 17, 2012 at 02:11:13PM +0200, Christophe Fergeau wrote:
> On Tue, Apr 17, 2012 at 02:51:33PM +0300, Alon Levy wrote:
> > On Tue, Apr 17, 2012 at 01:37:26PM +0200, Christophe Fergeau wrote:
> > > On Tue, Apr 17, 2012 at 01:46:26PM +0300, Alon Levy wrote:
> > > > On Tue, Apr 17, 2012 at 01:12:26PM +0300, Yonit Halperin wrote:
> > > > > ---
> > > > >  server/red_worker.c |    5 +++--
> > > > >  1 files changed, 3 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > > > index 07782c8..5350195 100644
> > > > > --- a/server/red_worker.c
> > > > > +++ b/server/red_worker.c
> > > > > @@ -11348,10 +11348,11 @@ static void dump_bitmap(RedWorker *worker, SpiceBitmap *bitmap, uint32_t group_i
> > > > >      }
> > > > >      /* writing the data */
> > > > >      for (i = 0; i < bitmap->data->num_chunks; i++) {
> > > > > +        int j;
> > > > 
> > > > Missed this in v1, sorry - shouldn't this be defined at the function
> > > > start? Not a big deal.
> > > 
> > > In my opinion, it's better to keep variable declaration in the smallest
> > > enclosing block.
> > 
> > I agree it's better. I commented out of a misplaced thought we should
> > support C-something compilers that don't allow it. I take it back.
> 
> Oh, declaring variables at the beginning of a C block as is done here is
> supported by all C compilers I know of. It's declaring variables in the
> middle of a block (after some non-declarations lines of code) which is not
> supported by all compilers. In this case, I don't think this will cause any
> issues.

"don't allow it" - it being ~it from your last sentence.

> 
> Christophe



> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel