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

[David Kastrup]

Han-Wen Nienhuys <address@hidden> writes:

> On Tue, Aug 16, 2011 at 7:17 PM, David Kastrup <address@hidden> wrote:
>>> (and I am speaking as a GUILE developer here as well)
>>
>> So what does relying on undefined behavior buy us apart from the
>> inability to debug type errors?
>
> It buys us time to work on more interesting and more valuable
> improvements.

More time than you want.  And it drives away prospective contributors
who try to get a hang of Lilypond and Guile, read the code and the
respective docs, and find that they need to understand all the internals
of Guile rather than its documented API before being able to understand
what code might and might not be in error, and what code might or might
not make the grownups laugh at them because it looks way more carefully
written than the rest.

I mean, look at the bad code I dug up.  Pretty early in the list there was:

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

This line was committed in 2005 by Jan in its original state.  Why would
Jan add a check against SCM_UNDEFINED before checking for a string?
Most likely because he tried it out and was surprised that an undefined
value passed for a string, and then fixed this curiosity by explicitly
checking for undefined.

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

-- 
David Kastrup