Re: [pdf-devel] CCITT Fax Filter

[Aleksander Morgado]

> > > There's a problem with the trunk. After performing 'make' I get:
> > > 
> > > pdf-filter.c: In function 'install_filters':
> > > pdf-filter.c:1317: error: 'JBIG2DEC_GLOBAL_SEGMENTS_ARG' undeclared
> > > (first use in this function)
> > > pdf-filter.c:1317: error: (Each undeclared identifier is reported only
> > > once
> > > pdf-filter.c:1317: error: for each function it appears in.)
> > > pdf-filter.c:1317: error: 'JBIG2DEC_PAGE_SIZE' undeclared (first use in
> > > this function)
> > > 
> > > It seems that macro IS_FILTER_ARG compares with JBIG2DEC even when it's
> > > not declared. I send a patch.
> > 
> > Yes, I can confirm this problem (if no HAVE_LIBJBIG2DEC). Your patch is
> > ok, but we would need an extra macro for each further configuration
> > option with arguments (and combinations). Are any further configuration
> > option with arguments planed?
> > 
> > Maybe it is better to declare all values inside the enum (before the
> > macro), or to make a function instead of the macro (so #ifdef can be
> > used inside). But this can be done later too, so decide for yourself.
> > 
> > Regards,
> > Georg
> > 
> > -----BEGIN PGP SIGNATURE-----
> > Version: GnuPG v2.0.17 (GNU/Linux)
> > Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
> > 
> > iEYEARECAAYFAk6GSDUACgkQ5sLITM1qIaKpPgCfWCDHzE8ueUyBpddgtzrXRQKd
> > 4eEAnR9Itm/A2CTDbQv0KxZa2rn687t+
> > =xAIr
> > -----END PGP SIGNATURE-----
> 
> Yes, in case others were planned it would become a mess. So I settle for
> the function idea. Here it goes.
> 

Please attach always uncompressed files, with .diff or .txt extensions;
it is much easier to review the patches inline.

Just some coding style comments...

> +static pdf_bool_t
> +is_filter_arg(enum filter_arg arg)
Whitespace needed before the parenthesis.

> +  return (arg) == PRED_COLORS_ARG 
> +    || (arg) == PRED_BITSPERCOMPONENT_ARG    
> +    || (arg) == PRED_COLUMNS_ARG
> +    || (arg) == PRED_PREDICTOR_ARG

I don't usually split the lines with multiple conditions before the
operator, but the GCS suggests to do it. I would add an extra
parenthesis to group all conditions, remove the parenthesis around
'arg' (not a macro anymore), and get all conditions aligned, like:
   return (arg == PRED_COLORS_ARG
           || arg == PRED_BITSPERCOMPONENT_ARG
           || arg == PRED_COLUMNS_ARG
           || arg == PRED_PREDICTOR_ARG
           ...);

> +      if (!is_filter_arg(next_ci) && filter_to_install != 
> FILTER_INSTALL_NONE)
whitespace needed before parenthesis.

Cheers!

-- 
Aleksander

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