On Sun 09 Dec 2012 13:47, Andreas Rottmann <a.rottm...@gmx.at> 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/
