Re: [PATCH 2/3] Add internal API to specify reader options at reader invocation

[Andy Wingo]

On Sun 09 Dec 2012 13:47, Andreas Rottmann <address@hidden> writes:

> * libguile/private-options.h: Introduce a new enum indexing the read
>   options, and use its values as indices for scm_read_opts.

Seems to define struct scm_read_opts as well?

> +SCM scm_i_read (SCM port, const scm_t_read_opts *opts, unsigned int preset)

Comment needed about the role of "preset"

> @@ -2191,8 +2173,8 @@ set_port_read_option (SCM port, int option, int 
> new_value)
>      read_options = scm_to_uint (scm_read_options);
>    else
>      read_options = READ_OPTIONS_INHERIT_ALL;
> -  read_options &= ~(READ_OPTION_MASK << option);
> -  read_options |= new_value << option;
> +  read_options &= ~(READ_OPTION_MASK << (option * 2));
> +  read_options |= new_value << (option * 2);

This is getting super-nasty.  Some kind of abstraction is needed here,
perhaps a static function.

> @@ -2244,28 +2226,29 @@ init_read_options (SCM port, scm_t_read_opts *opts)
>    else
>      read_options = READ_OPTIONS_INHERIT_ALL;
>  
> +  if ((preset & (1 << SCM_READ_OPTION_KEYWORD_STYLE)) == 0) {
> +    x = READ_OPTION_MASK & (read_options >> (SCM_READ_OPTION_KEYWORD_STYLE * 
> 2));

Why is this option special?  (I have a guess, but a comment seems to be
lacking)

>  
> +typedef struct scm_struct_read_opts scm_t_read_opts;

No need to infix "struct" into the name; struct tag namespaces are
disjoint from type namespaces.

I have no idea if this patch is good or not; just a drive-by.

Andy
-- 
http://wingolog.org/