Re: [Chicken-hackers] [PATCH][5] Some FFI improvements

[Peter Bex]

On Sun, May 28, 2017 at 11:29:06PM +0200, lemonboy wrote:
> Hello hackers,

Hi Lemonboy,

Thanks (again!) for your patches.  You're really putting in quite
the effort.

> I'll be brief:
> - The first patch fixes a problem where we'd fail to consider the
> internal defines as toplevel ones,
>  leading to a compiler error.
> - The second patch complements the first by rejecting more `define-`
> forms in non-toplevel contexts.

Those two are pretty straightforward, so I attached signed-off copies
(minus the whitespace changes).

> - The third one is slightly beefier as it prevents us to apply the
> wrong specialization in some cases
>  like in the following snippet.
> 
> ```
> (import foreign)
> (define-foreign-type ty int (lambda (x) 42) (lambda (x) 0.5))
> (let ((t0 ((foreign-lambda* ty ((ty x)) "return 0;") 'sym)))
>   (print (min t0 1))) ;; We expect this to be 0.5 and not 1
> ```

I have two comments about the patch, and a more generic comment:

1) I don't like the extra "mode" argument making its way into the
    final-foreign-type API.  It has nothing to do with the _foreign_
    type of the value, that's unchanged.  This is purely a scrutinizer
    change, and it shouldn't "infect" code that deals only with foreign
    types.

2) Your patch bails out if it encounters any conversion functions during
    the lookup.  However, this is overly strict: the procedures
    foreign-type-convert-{result,argument} will perform a "shallow"
    lookup.  That means if you create a foreign type "alias" for another
    type, the alias won't inherit the type converters of the other type.
    Whether that's desirable is up for debate, of course, but that's how
    it currently works.  So we can just use the "final" type if the
    initial lookup returns no converters.

More generally, I think indiscriminantly adding (##core#the ...)
annotations for foreign values is too aggressive.  It would be better
to only do that when we can determine a proper result type.

Allowing an optional type annotation would allow the scrutinizer to
analyse the converter's type.  Currently, returning '* just blocks the
scrunitizer from performing its analysis.  In fact, it would be even
better if we can use the foreign type's scrutiny type as information
for the conversion procedures' input or output.  But that's just wishful
thinking.

For now, I think the attached patch will do, and has less impact because
the change is more localised.  I struggled quite a bit to make this code
readable, so if anyone knows how to improve this, please do!

Cheers,
Peter

0001-Do-not-treat-internal-defines-as-non-toplevel-ones.patch
Description: Text Data

0002-Reject-more-definitions-outside-of-toplevel-context.patch
Description: Text Data

0003-Fix-the-type-inference-with-foreign-types.patch
Description: Text Data

signature.asc
Description: Digital signature