Hi! After being puzzled for some time, I found the root cause
for an issue with my code was actually a quite serious bug in
SRFI-9's `define-inlinable' (here we go again -- but this time
it's an actual bug, I promise ;-)): The current implementation of
`define-inlinable' duplicates actual parameters passed to the
macros it generates, for example (note the `(find-next-solution! s
10000)' expression): scheme@(guile-user)> (tree-il->scheme
(macroexpand '(solution? (find-next-solution! s 10000))))
$6 = (if ((@@ (srfi srfi-9) struct?) (find-next-solution! s 10000))
((@@ (srfi srfi-9) eq?)
((@@ (srfi srfi-9) struct-vtable) (find-next-solution! s 10000))
(@@ (dorodango solver) solution))
#f)
Attached is a patch to fix this (along with a test case), but I guess
it's not as good as it could be:
- It doesn't preserve the current error-at-syntax-time behavior when an
argument count mismatch occurs (this makes the first two "constructor"
testcases in srfi-9.test fail, and obsolete). I don't know if this is
considered important.
- It's certainly not as efficient as the current, broken implementation.
Ideally, the expansion would be a `let' block instead of the
`call-with-values' invocation. I'd guess the former might be more
amendable for optimization, but perhaps I'm wrong here. I didn't
implement the solution using `let', as I believe it cannot be done in
a straightforward way (i.e. by exploiting the pattern matcher only)
with Guile's current syntax-case implementation.
Has anyone done benchmarks of the benefits of the current (broken)
SRFI-9 accessors and predicates vs. regular procedures on real code?
Given the argument duplication issue, it might be the case that
`define-inlinable' actually slowed things down :-).
Perhaps removing `define-inlinable' entirely would be an acceptable
alternative fix for this issue?
Anyway, here's the patch:
From: Andreas Rottmann <[email protected]>
Subject: Fix argument duplication `define-inlinable' in SRFI-9
* modules/srfi/srfi-9.scm (define-inlinable): Don't duplicate argument
expressions in the expansion of the macros generated.
* test-suite/tests/srfi-9.test (constructor): Adapted.
(side-effecting arguments): New test for the argument duplication
issue.
---
module/srfi/srfi-9.scm | 5 +++--
test-suite/tests/srfi-9.test | 22 +++++++++-------------
2 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/module/srfi/srfi-9.scm b/module/srfi/srfi-9.scm
index fad570b..34dbb53 100644
--- a/module/srfi/srfi-9.scm
+++ b/module/srfi/srfi-9.scm
@@ -87,8 +87,9 @@
(define-syntax name
(lambda (x)
(syntax-case x ()
- ((_ formals ...)
- #'(begin body ...))
+ ((_ . arguments)
+ #'(call-with-values (lambda () (values . arguments))
+ (lambda (formals ...) body ...)))
(_
(identifier? x)
#'proc-name))))))))))
diff --git a/test-suite/tests/srfi-9.test b/test-suite/tests/srfi-9.test
index f8006c4..6766482 100644
--- a/test-suite/tests/srfi-9.test
+++ b/test-suite/tests/srfi-9.test
@@ -38,21 +38,10 @@
(with-test-prefix "constructor"
- ;; Constructors are defined using `define-integrable', meaning that direct
- ;; calls as in `(make-foo)' lead to a compile-time psyntax error, hence the
- ;; distinction below.
-
- (pass-if-exception "foo 0 args (inline)" exception:syntax-pattern-unmatched
- (compile '(make-foo) #:env (current-module)))
- (pass-if-exception "foo 2 args (inline)" exception:syntax-pattern-unmatched
- (compile '(make-foo 1 2) #:env (current-module)))
-
(pass-if-exception "foo 0 args" exception:wrong-num-args
- (let ((make-foo make-foo))
- (make-foo)))
+ (make-foo))
(pass-if-exception "foo 2 args" exception:wrong-num-args
- (let ((make-foo make-foo))
- (make-foo 1 2))))
+ (make-foo 1 2)))
(with-test-prefix "predicate"
@@ -94,6 +83,13 @@
(pass-if-exception "set-y! on bar" exception:wrong-type-arg
(set-y! b 99)))
+(with-test-prefix "side-effecting arguments"
+
+ (pass-if "predicate"
+ (let ((x 0))
+ (and (foo? (begin (set! x (+ x 1)) f))
+ (= x 1)))))
+
(with-test-prefix "non-toplevel"
(define-record-type :frotz (make-frotz a b) frotz?
--
tg: (df12979..) t/srfi-9-fix2 (depends on: stable-2.0)
Regards, Rotty
--
Andreas Rottmann -- <http://rotty.yi.org/>
