Re: [Chicken-hackers] [PATCH][5] The other part of the FFI improvements

[Peter Bex]

On Mon, Jul 03, 2017 at 05:22:26PM +0200, lemonboy wrote:
> This patch offloads all the heavy lifting to the scrutinizer, most of it is 
> just
> boilerplate (that I hope I've got right) to introduce a new node to the core
> language named ##core#invoke (the name is up for discussion of course).
> The node serves as a simple way to represent the call of a ffi stub and it is 
> a
> nice mixture of ##core#call, ##core#primitive and ##core#inline that's easy to
> analyze during the scrutiny phase and is desugared into simpler code later in
> the pipeline.

Hi Lemonboy,

For the benefit of those not on IRC, I'll recap my comments from yesterday.

The patch's functionality is something that's definitely desirable, and many
thanks for actually putting in the effort after my off-hand comments about
how to improve this!

I'm a bit concerned about the patch itself though.  For one, in the interest
of modularity and code layout, I think all type annotations on variables
should _only_ be made by the scrutinizer.  It's the scrutinizer's job to
assign types, not anything else's.

There's of course the fact that the FFI knows the foreign types, and I think
it's fine to decorate the node tree with foreign types, or perhaps add a
node that contains the type (somehow use ##core#the in this situation?),
but straight up annotating the stub symbol with the types is not done, IMHO.
Having the foreign types in the node tree might even help us get rid of
some attributes in the ffi stubs table (though that's not really a goal).

The other thing I think may cause some headache in the future is the fact
that in the scrutinizer, ##core#invoke will cause a temporary ##core#call to
be faked out in order to leverage the existing code.  This is (potentially)
problematic, because everywhere else in the compiler, nodes are treated as
"final", and may be mutated at will.  By creating a throwaway temporary
node, this will cause any mutations on the node to be thrown away.

I think a proper solution will probably involve some pretty large
refactoring of either ##core#call (so the code can be re-used by
##core#invoke), or a refactoring of ##core#inline/##core#primitive
so that it becomes more "like" a ##core#call node and can contain
type information.

Given that this is a non-user visible change (at least, API-wise),
perhaps we should shelve this feature for 5.1?  It's not that important
of a change, and we have enough other things to worry about.

Cheers,
Peter

signature.asc
Description: PGP signature