[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Quite sneaky bug in SRFI-9

From: Andreas Rottmann
Subject: Quite sneaky bug in SRFI-9
Date: Thu, 10 Mar 2011 01:23:03 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux)

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))

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:

Attachment: srfi-9-fix2.diff
Description: Text Data

Regards, Rotty
Andreas Rottmann -- <http://rotty.yi.org/>

reply via email to

[Prev in Thread] Current Thread [Next in Thread]