useless verifications in r35_pack

Bug #323921 reported by zanin
2
Affects Status Importance Assigned to Milestone
Cuneiform for Linux
New
Undecided
Unassigned

Bug Description

This is about a cycle in the function r35_pack in the file r35.c. Of course, the same applies to the function r35_pack_gray.

for(t=kx=k=0;k<TO_Y;k++,kx+=TO_X)
430 {
431 for(num=Ycut[k]+1;num<Ycut[k+1];num++,t+=d_x)
 432 { /* first lines in strip */
433 compress_line( &rast[t],dx, buf_comp,TO_X,Xcut,Xval);
434 addcomp(&res_comp[kx],buf_comp,TO_X,TO_Y);
435 }
436 if( num!=dy )
437 { /* last line in strip */
438 compress_line( &rast[t],dx, buf_comp,TO_X,Xcut,Xval); t+= d_x;
439 if( Yval[k+1] )
440 { /* intrsected line */
441 ALL_addcomp(&res_comp[kx],buf_comp,TO_X,Yval[k+1]);
442 ALL_addcomp(&res_comp[kx+TO_X],buf_comp,TO_X,TO_Y-Yval[k+1]);
443 }
444 else /* alone line */
445 addcomp(&res_comp[kx+TO_X],buf_comp,TO_X,TO_Y);
446 }
447 }

Look at the very first if. Clearly, the value of num here is Ycut[k+1]. According to the definition of the array Ycut (which is given by the function Makescale), Ycut[k+1]!=dy if and only if k+1<TO_Y. Hence, we can mostly avoid this verification inside the cycle. This also allows us to get rid of some variables. I suggest to rewrite the cycle as the following:

for(k=0;k<TO_Y-1;k++)
 {
 for(num=Ycut[k]+1;num<Ycut[k+1];num++)
  { /* first lines in strip */
  compress_line( &rast[num*d_x],dx, buf_comp,TO_X,Xcut,Xval);
  addcomp(&res_comp[k*TO_X],buf_comp,TO_X,TO_Y);
  }
                 /* last line in strip */
  compress_line( &rast[num*d_x],dx, buf_comp,TO_X,Xcut,Xval);
  if( Yval[k+1] )
   { /* intrsected line */
   ALL_addcomp(&res_comp[k*TO_X],buf_comp,TO_X,Yval[k+1]);
   ALL_addcomp(&res_comp[(k+1)*TO_X],buf_comp,TO_X,TO_Y-Yval[k+1]);
   }
  else /* alone line */
   addcomp(&res_comp[(k+1)*TO_X],buf_comp,TO_X,TO_Y);
   }

for(num=Ycut[k]+1;num<Ycut[k+1];num++)
  { /* first lines in strip */
  compress_line( &rast[num*d_x],dx, buf_comp,TO_X,Xcut,Xval);
  addcomp(&res_comp[k*TO_X],buf_comp,TO_X,TO_Y);
  }

Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

Having additional sanity checks is not a bad thing unless they complicate the code needlessly.

Rather than getting rid of this completely, maybe we should convert it to an assert? This would document the invariant expected by the function.

Revision history for this message
zanin (zani0005) wrote :

These are not sanity checks. The array Xcut behaves in a very predictable manner. This check just overcomplicates the code without doing anything useful.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.