Hi,
On Wed 27 Mar 2013 21:00, Mark H Weaver <m...@netris.org> writes:
> index 8737a76..45e770d 100644
> --- a/libguile/ports.c
> +++ b/libguile/ports.c
> @@ -60,6 +60,8 @@
> #include "libguile/fluids.h"
> #include "libguile/eq.h"
>
> +#include "libguile/private-ports.h"
> +
> #ifdef HAVE_STRING_H
> #include <string.h>
> #endif
> @@ -589,10 +591,10 @@ finalize_port (void *ptr, void *data)
>
> entry = SCM_PTAB_ENTRY (port);
>
> - if (entry->input_cd != (iconv_t) -1)
> - iconv_close (entry->input_cd);
> - if (entry->output_cd != (iconv_t) -1)
> - iconv_close (entry->output_cd);
> + if (entry->internal->input_cd != (iconv_t) -1)
> + iconv_close (entry->internal->input_cd);
> + if (entry->internal->output_cd != (iconv_t) -1)
> + iconv_close (entry->internal->output_cd);
>
> SCM_SETSTREAM (port, 0);
> SCM_CLR_PORT_OPEN_FLAG (port);
I would appreciate a symmetry between the name of the internal member
and the name of the header. WDYT about "internal" in both?
I also think that "ports-internal.h" is a better name than
"internal-ports.h".
> + entry->internal = (struct scm_t_port_private *)
> + scm_gc_calloc (sizeof (struct scm_t_port_private), "port-private");
Would be nice to have a macro to allocate structs, with the correct
type. Like
#define scm_gc_typed_calloc(t) (((t)*) scm_gc_calloc (sizeof (t), #t))
> @@ -1297,12 +1307,14 @@ get_iconv_codepoint (SCM port, scm_t_wchar *codepoint,
> char buf[SCM_MBCHAR_BUF_SIZE], size_t *len)
> {
> scm_t_port *pt;
> + struct scm_t_port_private *pti;
Indeed here the name belies your intention ;) Seems you want to call the
type scm_t_port_internal. (Though, do we need _t_ in struct tags?
Seems redundant, given that struct tags are a separate namespace.
Dunno)
> @@ -1332,7 +1344,7 @@ get_iconv_codepoint (SCM port, scm_t_wchar *codepoint,
> input_left = bytes_consumed + 1;
> output_left = sizeof (utf8_buf);
>
> - done = iconv (pt->input_cd, &input, &input_left,
> + done = iconv (pti->input_cd, &input, &input_left,
> &output, &output_left);
Would be nice to fix up tabs here
> @@ -1369,15 +1381,22 @@ get_codepoint (SCM port, scm_t_wchar *codepoint,
> int err;
> scm_t_port *pt = SCM_PTAB_ENTRY (port);
>
> - if (pt->input_cd == (iconv_t) -1)
> + if (pt->internal->encoding_type == SCM_PORT_ENCODING_TYPE_UNINITIALIZED)
Here I would appreciate consonance with the SCM_PORT_ENCODING_MODE in
`master'.
> + struct scm_t_port_private *pti;
> iconv_t new_input_cd, new_output_cd;
> + enum scm_t_port_encoding_type new_encoding_type;
In general we use typedefs for enums in guile, fwiw
So currently this patch is not OK for me, because of the interaction
with master. Can you take a look at porting
6c98257f2ead0855f218369ea7f9a823cdb9727e?
This work you are doing will not be easy to merge forward. If you are
willing to take care of that, I think I will be OK with something going
into stable-2.0 after this review is over. If not, I would prefer some
other solution (for example one of the 3 or 4 BOM-related patches I made
that you rejected ;-)
Thanks,
Andy
--
http://wingolog.org/
