Re: [pdf-devel] CCITT Fax Filter

[Aleksander Morgado]

> I completed the Group 3 1D encoder and decoder. It shouldn't be
> difficult (hope) now to create the 2D from that one, which I'll do next.
> I send it in case someone wants to give it a try (and test it more, that
> is something I'd like).
> 
> 
> Regards,
> Gustavo
> 
> PS: It's base on Aleks' stream-api branch.

Why is the COPYING file being removed in your patch?
Why are there hash-related unit tests being included in the patch?
Can you sync the patch with trunk and let it contain only your relevant
changes? It will be much easier to review.

As a general note, please try to follow the same coding style as all
(most of) the other source files; I've spent a lot of time trying to
give a common coding style lately...

Specifically:

Don't use tabs to indent, please; we do spaces only. Seems you end up
mixing tab and spaces indentation in several places.

Are these commented lines intended? If so, please add some comment
explaining why and also use /* */ to comment the code. This also applies
to multiple places in the patch.
+  union {
+    decoder_g31d_t *g31d;
+    //encoder_g32d_t *g32d;
+    //encoder_g4_t *g4;
+  } data;

Please always include whitespace before the parenthesis in method
definitions and calls, e.g. don't do:
+encoder_find_span(pdf_bit_buffer_t *buf, int max_len, pdf_bool_t ones)

Please follow the GCS for the use of parenthesis, alignment and such; so
don't do:
   while (something) {
       foo;
   }
Instead, it would be:
   while (something)
     {
       foo;
     }

Instead of these debugging #ifdefs:
+#ifdef G31DENCODER_DEBUG
+       fprintf(stderr, "encode_g31d_aplly: init\n");
+#endif
You can use PDF_DEBUG_BASE() and compile with --enable-debug-base. I
think it will be much clearer.

Give one arg per line in function definitions/declarations, and align
them with spaces, no tabs. I usually also align arg names, although not
that big deal. So, instead of:
+encoder_put_bits(pdf_bit_buffer_t *buf, pdf_u16_t bits, int nbits)
Better:
+encoder_put_bits (pdf_bit_buffer_t *buf,
                   pdf_u16_t         bits,
                   int               nbits)

I didn't really check what the patch does itself; got pain in the eye
with so many coding style things :-).

Could you update the patch with these comments, and we keep reviewing
it? It really is much easier to review and maintain if all the code uses
the same coding style.

Thanks!

-- 
Aleksander

signature.asc
Description: This is a digitally signed message part