Hi Mark,
Thanks for the review!
On Sun 27 Apr 2014 18:00, Mark H Weaver <m...@netris.org> writes:
> Andy Wingo <wi...@pobox.com> writes:
>
>> Here is the proposed C API:
>>
>> SCM scm_make_foreign_object_type (SCM name, SCM slot_names,
>> scm_t_struct_finalize finalizer);
>
> Shouldn't it be 'scm_t_struct_finalizer', with trailing 'r'?
Probably it should, but note that it's already defined -- see struct.h.
(My bad there, I think!)
>> SCM scm_make_foreign_object_n (SCM type, size_t n, scm_t_bits vals[]);
>>
>> scm_t_bits scm_foreign_object_ref (SCM obj, size_t n);
>> void scm_foreign_object_set_x (SCM obj, size_t n, scm_t_bits val);
>
> I'm worried about the type-punning that's implied by this interface.
This issue exists already with the SMOB and struct interfaces. I don't
think that the proposed API adds new hazards, though it does extend the
old ones.
> The C standards are now very strict about this sort of thing,
It's an incredible shame that C is making itself semantically less
capable as time goes on :-(
> For example, having recently researched this, I know that converting
> unsigned integers to signed integers is very awkward and potentially
> inefficient to do portably, so using 'scm_foreign_object_ref' to extract
> a signed integer will be a pain. It would look something like this
> (untested):
>
> scm_t_signed_bits
> scm_foreign_object_signed_ref (SCM obj, size_t n)
> {
> scm_t_bits bits = scm_foreign_object_ref (obj, n);
>
> /* GNU C specifies that when casting to a signed integer of width N, the
> value is reduced modulo 2^N to be within range of the type. Neither
> C99 nor C11 make any guarantees about casting an out-of-range value
> to a signed integer type. */
>
> #ifndef __GNUC__
> if (bits > SCM_T_SIGNED_BITS_MAX)
> return -1 - (scm_t_signed_bits) ~bits;
> #endif
> return (scm_t_signed_bits) bits;
> }
True sadness.
> Portability is more problematic for pointer types. The C standards make
> no guarantees about the semantics of converting between pointers and
> integers, and it's not clear to me how future proof this will be.
Don't they make some guarantees wrt uintptr_t and intptr_t ?
> One related issue is that I'd like to leave open the possibility that
> 'scm_t_bits' might be larger than a pointer.
You prefer to have three interface then: one for uintptr_t, one for
intptr_t, and one for void*?
> For constructors, I think we should provide a macro that handles the
> conversion from pointer to scm_t_bits. (Converting from signed to
> unsigned is portable, so there's no issue there).
This is really gross...
>> The overhead of a foreign object is two words -- the same as the
>> overhead on any struct. (Compare to SMOBs, which have a half-word
>> overhead.)
>
> It would be good to think about how we might someday reduce this
> overhead in the future, and to make sure we don't make decisions that
> would prevent us from doing that.
This is an orthogonal issue, I think, and would like to punt on that for
now.
>> + scm_t_bits vals[2] = { val0, val1 };
>
> This non-constant initializer depends on C99.
Will fix.
>> + if (scm_i_symbol_ref (layout, i * 2) != 'u')
>> + scm_wrong_type_arg_msg (FUNC_NAME, 0, layout, "'u' field");
>
> This looks inefficient. How about using 'scm_i_symbol_chars'?
OK.
Cheers,
Andy
--
http://wingolog.org/
