Re: Likely a good frog project for someone with C knowledge

[Han-Wen Nienhuys]

On Wed, Aug 17, 2011 at 5:19 AM, David Kastrup <address@hidden> wrote:

> --- a/lily/general-scheme.cc
> +++ b/lily/general-scheme.cc
> @@ -381,7 +381,7 @@ LY_DEFINE (ly_stderr_redirect, "ly:stderr-redirect",
>   string m = "w";
>   string f = ly_scm2string (file_name);
>   FILE *stderrfile;
> -  if (mode != SCM_UNDEFINED && scm_string_p (mode))
> +  if (scm_is_string (mode))
>
> Why would anybody write a condition like
>  if (mode != SCM_UNDEFINED && scm_string_p (mode))
> if he did not first try just scm_string_p (mode), could not figure out
> why it didn't work, and then added a check against SCM_UNDEFINED?

SCM_UNDEFINED is used to indicate a missing optional argument.  If you
look at general-scheme.cc that should be pretty obvious, because
pretty much every function looks like that.

I have spent 10 minutes looking in vain through the documentation to
find the "official" solution for optional arguments in the C
interface, but haven't been able to find it. In guile there are a
bunch of macros in validate.h, but they do not appear in the
documentation.

Oh, I just found it now.

http://www.gnu.org/software/guile/manual/html_node/API-Overview.html#API-Overview

"For some Scheme functions, some last arguments are optional; the
corresponding C function must always be invoked with all optional
arguments specified. To get the effect as if an argument has not been
specified, pass SCM_UNDEFINED as its value. You can not do this for an
argument in the middle; when one argument is SCM_UNDEFINED all the
ones following it must be SCM_UNDEFINED as well."

I guess we could use

 SCM_UNBNDP()

instead.  One thing I dislike about converting to the 'official' API
is that it will litter our source-code with equally inscrutable
macros.  Perhaps we should just roll our own?

  inline SCM ly_is_unbound(SCM)

looks much nicer to me.

>> Speaking as a long-time lilypond developer, it is my experience that
>> the errors you point out are not a problem (except for the SCM => bool
>> conversion).  GUILE's API uses data that can be passed into C
>> functions efficiently as parameters.  This means that the SCM type
>> must be a machine word, so the genericity suggested by the GUILE docs
>> are a joke.
>
> Except that there are insiders and outsiders to the joke.  Do you really
> consider yourself infallible so that you refuse to write code that would
> be recognizably correct for someone doing a code review?
>
> Code reviews are not fun.  And exactly because you want to buy yourself
> time to work on more interesting and more valuable improvements, you
> should strive to write code that can be reviewed by lesser people.
> Those that actually read the APIs of Guile.  Or in this case, write code
> that can be reviewed for correctness by the compiler.  The compiler is
> not overly eager to write more interesting and more valuable
> improvements, but it is a rather thorough and experienced reviewer.
>
>> If you feel compelled to change large swaths of source code by
>> substituting x == SCM_EOL with scm_is_eq(x, SCM_EOL), then I can't
>> stop you, but to me it just looks like a waste of time.
>
> That would be scm_is_null (x).  It is not exactly like the code gets
> less readable by that substitution.

it's not the same though. scm_is_null expands to

pairs.h:#define scm_is_null(x)          (scm_is_null_or_nil(x))

on the plus side, if we use this, we will be the first GNU program to
be compatible with the elisp compatibility mode in GUILE that has been
almost ready for the last 15 years.

--
Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen