Fix formatting in GIF2.cpp

RESOLVED FIXED in mozilla1.3alpha

Status

()

Core
ImageLib
--
trivial
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: paper, Assigned: paper)

Tracking

Trunk
mozilla1.3alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Updated

16 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

16 years ago
Summary: Change GIF2.cpp to tab-width 2 → Fix formatting in GIF2.cpp
Target Milestone: --- → mozilla1.3alpha
(Assignee)

Comment 1

16 years ago
Created attachment 104248 [details] [diff] [review]
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 on attachment 104248 [details] [diff] [review]
Cleanup

can you attach a diff -w?
(Assignee)

Comment 3

16 years ago
Created attachment 104280 [details]
diff -u2 -w

still rather long, but easier to find out what I did.
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?
(Assignee)

Comment 5

16 years ago
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
-                }
-            }  
+      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.
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.
  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.
(Assignee)

Comment 9

16 years ago
Created attachment 105616 [details] [diff] [review]
Clean-up more
Attachment #104248 - Attachment is obsolete: true
(Assignee)

Comment 10

16 years ago
Created attachment 105617 [details]
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.
Attachment #104280 - Attachment is obsolete: true
(Assignee)

Comment 11

16 years ago
Created attachment 105625 [details] [diff] [review]
Super Clean-Up

more clean-up as per discussion with biesi on IRC
Attachment #105616 - Attachment is obsolete: true
(Assignee)

Comment 12

16 years ago
Created attachment 105626 [details]
diff -u2 -w
Attachment #105617 - Attachment is obsolete: true

Comment 14

16 years ago
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
Attachment #105625 - Flags: superreview+
(Assignee)

Comment 15

16 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.