[Spice-devel] [spice-server PATCH 1/3] compress_seg: comment out unused assignment

[Frediano Ziglio]

> 
> > 
> > CLANG warning: "Value stored to 'ref_limit' is never read"
> > 
> > Commenting out since there is a ToDo that refers to ref_limit
> > 
> > Found by Covscan.
> > 
> > Signed-off-by: Uri Lublin <uril at redhat.com>
> > ---
> > 
> > Should the comment be deleted too ?
> > 
> 
> I think would be time to revise the comment and maybe the code.
> 
> The comment is (I suppose):
>     while (ip < ip_bound) { // TODO: maybe separate a run from the same seg
>     or from
>                            // different ones in order to spare ref <
>                            ref_limit
> 
> but this came from commit:
> commit c1b79eb035fa158fb2ac3bc8e559809611070016
> Author: Yaniv Kamay <ykamay at redhat.com>
> Date:   Sat Sep 19 21:25:46 2009 +0300
> 
>     fresh start
> 
> with no further history :-(
> Is the former history somewhere?
> 
> Something is suggesting me that the loop was "while (ref < ref_limit)"
> instead of the current "while (ip < ip_bound)". Also wondering why
> ip_bound is ref_limit (seg->lines_end) minus 2 pixels and so why RLE
> check ignores last 2 pixels. But I have similar questions for ip_limit.
> 
> > ---
> >  server/glz-encode.tmpl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/server/glz-encode.tmpl.c b/server/glz-encode.tmpl.c
> > index 48bab50bc..ad48c86c8 100644
> > --- a/server/glz-encode.tmpl.c
> > +++ b/server/glz-encode.tmpl.c
> > @@ -282,7 +282,7 @@ static void FNAME(compress_seg)(Encoder *encoder,
> > uint32_t seg_idx, PIXEL *from,
> >  
> >                  ip += 3;
> >                  ref = anchor + 2;
> > -                ref_limit = (PIXEL *)(seg->lines_end);
> > +                //ref_limit = (PIXEL *)(seg->lines_end);
> >                  len = 3;
> >  
> >                  x = *ref;
> 

I realized that GLZ is just LZ with the dictionary shared between
multiple images. So I looked at LZ code (spice-common) to find the
missing clues.
In the LZ code that part is used for both RLE and dictionary. So the
loop in LZ code (common/lz_compress_tmpl.c) is

    while ((ip < ip_bound) && (ref < ref_limit)) { // TODO: maybe separate a run from
                                                   //       the same seg or from different
                                                   //       ones in order to spare
                                                   //       ref < ref_limit

so here the comment make sense. However this was copied verbatim
in GLZ but ref < ref_limit check was removed as the same segment is used.
I suppose that the LZ code having ref == ip - 1 in the RLE case is comparing
an additional pixel leaving out only 1 pixel but this is OT for this case.
I'll write a patch (will be more time consuming to write the comment!).

Frediano