On Wed, Aug 17, 2011 at 5:19 AM, David Kastrup <d...@gnu.org> 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 - han...@xs4all.nl - http://www.xs4all.nl/~hanwen _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
