166007 - Fix formatting in GIF2.cpp

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[ArronM (:paper)]

Making patches for GIF2.cpp bugs me to no end because it's abnormally set to
tab-width 4.  tab-width should be 2.

Comment 1

[ArronM (:paper)]

Attachment: Cleanup

- 0 code changes. 

- 4 space tabs changed to 2 space tabs

- Removed all ILTRACEs (they were all commented out and were for old gifcom
stuff)

- Removed debug defines (again, only used way back in gifcom days)

- Placed return value type & function name on same line (ie "void^pfoo" to
"void foo()")

- moved { to same line as if, and "} else {"

- Removed trailing spaces

- Split any lines > 80 characters.

All this chopped 200+ lines off the file and made it much more consistent and
standardized to the rest of imglib. :)

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 104248 [details] [diff] [review]
Cleanup

can you attach a diff -w?

Comment 3

[ArronM (:paper)]

Attachment: diff -u2 -w

still rather long, but easier to find out what I did.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 104280 [details]
diff -u2 -w

what's this?
+      // q[6] = Pixel Aspect Ratio
+      //   Not used
+      //   float aspect = (float)((q[6] + 15) / 64.0);

why add this code?

Comment 5

[ArronM (:paper)]

The code below wasn't doing anything, so I killed it, but I thought the
information in it might be usefull in the future. (ie. someone asking why we
skipping over [6] or something like that)

-            if (q[6])
-            {
-                /* should assert gif89 */
-                if (q[6] != 49)
-                {
-#ifdef DEBUG
-                    float aspect = (float)((q[6] + 15) / 64.0);
-                    //ILTRACE(2, ("il:gif: %f aspect ratio", aspect));
-#endif
-                }
-            }  

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

+      if ( !gs->prefix || !gs->suffix || !gs->stack) {
           ^
remove the space here please
(other places like this as well)

+      if (strncmp((char*)q,"GIF",3)) {
space after comma, please
same for this one (which you didn't touch):
             GETN(7,gif_global_header);
and possibly in other places of that file.

-#ifdef DEBUG_saari
given the amount of checkins saari has done recently, I suppose that's ok.

+      if (*q!=',') {
space before and after != please.

+      for (int i=0; i < gs->local_colormap_size; i++, map++) {
space around = please

+          // XXX: we don't freeze decoder anymore
um, is the non-freezing a bad thing?

doesn't sound like one to me; and if it's good, it should not be XXX

ok the rest looks good, but I still have to apply it and test it.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

ok I now applied the patch and looked at the resulting file. I found a few
things, I hope you don't mind :) :
          /* PR_ASSERT(0); */
you probably should remove that comment (or replace with an NS_NOTREACHED)

    } while(gs->irow > gs->height - 1);
personally, I'd like parens around the second half of the comparison...

#define OUTPUT_ROW(gs)

this macro could use NSPR_BEGIN_MACRO / NSPR_END_MACRO

  for (ch=q; count-- > 0; ch++)
spaces around =

  gif_struct *ret;

  ret = PR_NEWZAP(gif_struct);

combine these lines?
actually, maybe assign it directly into the out parameter? (or does code depend
on an unchanged parameter in case it would be zero?)
hm, not sure if this really belongs in this bug...

  PR_ASSERT(gs);
can you replace that with NS_ASSERTION(gs, "Got null argument, will crash!");

  GIF_IRGB *img_trans_pixel;
I'd move it inside the if(), to where it's first used.

...gotta go now, will continue later. for my own reference, I'm near line 561 now.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

  if (gs->gathered < max)
that if condition can be written as
  return (gs->gathered < max);

PRStatus gif_write(gif_struct *gs, const PRUint8 *buf, PRUint32 len)
well I wish that would use nsresult but that does not really belong in this bug.

  //  PR_ASSERT ((len == 0) || (gs->gathered < MAX_READ_AHEAD));
guess you can remove this comment

  const PRUint8 *q, *p=buf, *ep=buf+len;
again, spaces around =. I'll not mention further cases of this, just fix it
everywhere.

  if (!((len == 0) || (gs->gathered < MAX_READ_AHEAD)))
now this line looks, uh, rather ugly. maybe change it to:
  if ((len != 0) && (gs->gathered >= MAX_READ_AHEAD))
(wow, I really hope I got the boolean algebra correct here...)

actually, for all the *(q + 8), you could write q[8] instead. that's better, imho.

        if ((num_colors > gs->local_colormap_size) &&
          gs->local_colormap)
        {
          PR_FREEIF(gs->local_colormap);
          gs->local_colormap = NULL;
        }
now this piece of code is interesting (as mentioned on irc)
in short, change the FREEIF to PR_Free, because FREEIF would also check for
non-nullness of local_colormap, and set it to NULL afterwards.
alternatively, remove the non-nullcheck from the if and the assignment of null.

now I see the NULL in this code... please do a global search-and-replace for
NULL -> nsnull.

      GIF_RGB* map;
      map = gs->local_colormap;

combine these two lines please.

      return PR_FAILURE;
      break;

no need for the break;s here. also in surrounding code.

ah yeah, and here (at the end of the file):
    if (gs->local_colormap) {
      PR_FREEIF(gs->local_colormap);
      gs->local_colormap = NULL;
    }

you can also replace all these lines with the PR_FREEIF or change the PR_FREEIF
to PR_Free


great, I'm done.

Comment 9

[ArronM (:paper)]

Attachment: Clean-up more

Comment 10

[ArronM (:paper)]

Attachment: diff -u2 -w

> +	     // XXX: we don't freeze decoder anymore
I removed that whole chunk.  We haven't frozen loading GIF in Mozilla since..
well, since the time before Mozilla was public.


> +	 if (strncmp((char*)q,"GIF",3)) {
> space after comma, please
> same for this one (which you didn't touch):
>	       GETN(7,gif_global_header);
Done, plus all similar


>	    /* PR_ASSERT(0); */
chopped :)


>     } while(gs->irow > gs->height - 1);
> personally, I'd like parens around the second half of the comparison...
Added parens (didn't remove old ones, they seem to be standard on while).  Also
added a space between while and (, and did that to other areas of code with a
while(, switch(, or return(.


> #define OUTPUT_ROW(gs)
> this macro could use NSPR_BEGIN_MACRO / NSPR_END_MACRO
Added PR_BEGIN/END_MACRO to this one, and removed NS from the other one.


> ret = PR_NEWZAP(gif_struct);
I'm going to leave this as is.	It does seem constructed a bit weird though.


>  GIF_IRGB *img_trans_pixel;
>I'd move it inside the if(), to where it's first used.

That whole function (gif_init_transparency) is a mess and could probably be cut
in half if it weren't for my theory that the function serves no purpose
anymore.  I've made a note to look deeper into my theory, and will leave the
code as is here.


I kept the PR_FREEIF, and removed the null check prior to it and the setting to
null after it.

Anything else I didn't mention, I changed as per comments.

Comment 11

[ArronM (:paper)]

Attachment: Super Clean-Up

more clean-up as per discussion with biesi on IRC

Comment 12

[ArronM (:paper)]

Attachment: diff -u2 -w

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 105625 [details] [diff] [review]
Super Clean-Up

r=biesi

Comment 14

[tor]

Comment on attachment 105625 [details] [diff] [review]
Super Clean-Up

Somebody set us up the bomb.  All your GIF2.cpp are belong to Paper!

sr=tor

Comment 15

[ArronM (:paper)]

Checking in GIF2.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/GIF2.cpp,v  <--  GIF2.cpp
new revision: 1.35; previous revision: 1.34
done

Marking Fixed