Re: [PATCH] Add private port structure, and move iconv descriptors there

[Andy Wingo]

Hi,

On Wed 27 Mar 2013 21:00, Mark H Weaver <address@hidden> 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/