Re: [PATCH] Add internal-only port structure; move iconv descriptors there

[Mark H Weaver]

Hi Ludovic,

address@hidden (Ludovic Courtès) writes:

> Mark H Weaver <address@hidden> skribis:
>
>> I've come to the conclusion that it is not safe to modify 'scm_t_port'
>> in 2.0 at all; not even to change the member names.  In brief, the
>> reason has to do with the C11 standard definition of "compatible types",
>> which ties into the strict aliasing rules.  Section 6.2.7 of C11 spells
>> out what it means for two structures declared in separate translation
>> units to be compatible, and among other things their member names must
>> be the same.
>
> I can’t imagine how changing the *name* of a member could change
> something to the structure’s layout in practice.

It doesn't change the structure's layout.  However, it could cause
link-time optimization to break our code.  Basically, when linking
together objects built with the old scm_t_port and the new scm_t_port,
it's feasible that the compiler could compare the two structures by tag
name, member names, etc, to determine if they are "compatible types"
according to section 6.2.7.  If they are not compatible types, then the
linker is allowed to assume that pointers to these two distinct types
cannot alias each other, and make optimizations appropriately.  Of
course this is a simplification of the detailed rules (and TBH I haven't
yet taken the time to fully understand the rules myself), but from
reading C11, I can see a potential for real problems in practice.

>> +#define scm_gc_typed_calloc(t) ((t *) scm_gc_calloc (sizeof (t), #t))
>
> Not really convinced by this, but hey.  Ideally, this would need to go
> in the manual too.

Maybe talk to Andy about it?  It was his suggestion.

>> +typedef struct scm_t_port_internal
>> +{
>> +  /* input/output iconv conversion descriptors */
>> +  void *input_cd;
>> +  void *output_cd;
>> +} scm_t_port_internal;
>
> Please define the struct tag and typedef separately.  Also, I’d remove
> ‘_t’ from the struct tag, as discussed before.

Okay.  I was just following the conventions used in current Guile code,
e.g. in ports.h, but I'll change it as you suggest.

      Thanks,
        Mark