Re: [pdf-devel] CCITT Fax Filter

[Gustavo Martin Domato]

On Sat, 2011-10-01 at 19:22 +0200, Aleksander Morgado wrote:
> > 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!
> 

Sorry about all that. I'll correct it and resend.


Regards,
Gustavo.