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
From 00e97b8b532c80ffbe02c1291d2e96b5bc5401d4 Mon Sep 17 00:00:00 2001 From: LemonBoy <thatle...@gmail.com> Date: Sun, 28 May 2017 22:12:32 +0200 Subject: [PATCH 1/3] Do not treat internal defines as non-toplevel ones We now treat the internal defines as if they're defined at the same level the define-foreign-type is. Signed-off-by: Peter Bex <pe...@more-magic.net> --- core.scm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core.scm b/core.scm index e73d207..e87815d 100644 --- a/core.scm +++ b/core.scm @@ -1221,7 +1221,7 @@ (define ,ret ,(if (pair? (cdr conv)) (second conv) '##sys#values)) ) - e se dest ldest h ln #f) ) ] + e se dest ldest h ln tl?))] [else (register-foreign-type! name type) '(##core#undefined) ] ) ) ) -- 2.1.4
From 99a461409be899191e3425285f9901da0755dff7 Mon Sep 17 00:00:00 2001 From: LemonBoy <thatle...@gmail.com> Date: Sun, 28 May 2017 22:33:47 +0200 Subject: [PATCH 2/3] Reject more definitions outside of toplevel context Since we use global tables to store those definitions it makes sense to restrict those to the toplevel only. Signed-off-by: Peter Bex <pe...@more-magic.net> --- core.scm | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/core.scm b/core.scm index e87815d..cc1dd9a 100644 --- a/core.scm +++ b/core.scm @@ -1195,6 +1195,11 @@ (name (if (pair? (cdddr x)) (fourth x) (symbol->string var)))) + (unless tl? + (quit-compiling + "~adefinition of foreign variable `~s' in non-toplevel context" + (if ln (sprintf "(~a) - " ln) "") + var)) (set! foreign-variables (cons (list var type (if (string? name) @@ -1207,6 +1212,11 @@ (let ((name (second x)) (type (strip-syntax (third x))) (conv (cdddr x))) + (unless tl? + (quit-compiling + "~adefinition of foreign type `~s' in non-toplevel context" + (if ln (sprintf "(~a) - " ln) "") + name)) (cond [(pair? conv) (let ([arg (gensym)] [ret (gensym)] ) -- 2.1.4
From ed4dd017d351b9d7de8ee0630fd257a9896b944b Mon Sep 17 00:00:00 2001
From: Peter Bex <pe...@more-magic.net>
Date: Mon, 29 May 2017 22:23:11 +0200
Subject: [PATCH 3/3] Fix the type inference with foreign types
We can't trust the returned values to be of the base type since we push
it through the conversion procedures.
Ideally we'd avoid adding explicit type annotations in this situation,
and let the scrutinizer deal with it, but that's a lot trickier to do
with the current code base.
---
support.scm | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/support.scm b/support.scm
index 2c04d2e..366bae2 100644
--- a/support.scm
+++ b/support.scm
@@ -1241,7 +1241,14 @@
;; Used only in chicken-ffi-syntax.scm; can we move it there?
(define (foreign-type->scrutiny-type t mode) ; MODE = 'arg | 'result
- (let ((ft (final-foreign-type t)))
+ ;; Take into account the fact we can't have a precise type if
+ ;; there are user-specified conversion procedures. It would be
+ ;; better if we could simply let the scrutinizer do its work on
+ ;; the procedure, instead.
+ (let* ((conv-slot (if (eq? mode 'arg) 1 2))
+ (ft-vec (and (symbol? t) (lookup-foreign-type t)))
+ (ft (and (or (not ft-vec) (not (vector-ref ft-vec conv-slot)))
+ (final-foreign-type t))))
(case ft
((void) 'undefined)
((char unsigned-char) 'char)
--
2.1.4
signature.asc
Description: Digital signature
_______________________________________________ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
