Hi Mark, This is also not a patch review yet :)
On Thu 09 Jul 2009 18:11, Mark H Weaver <m...@netris.org> writes: > I added the following macros, whose names explicitly state how %nil > should be handled. See the comments in the patch for more information > about these. > > scm_is_false_assume_not_lisp_nil scm_is_true_assume_not_lisp_nil > scm_is_false_and_not_lisp_nil scm_is_true_or_lisp_nil > scm_is_false_or_lisp_nil scm_is_true_and_not_lisp_nil > > scm_is_lisp_false scm_is_lisp_true > > scm_is_null_assume_not_lisp_nil > scm_is_null_and_not_lisp_nil > scm_is_null_or_lisp_nil > > scm_is_bool_and_not_lisp_nil > scm_is_bool_or_lisp_nil These are terrible names. But they seem to be the best names for the concepts we're trying to express. I don't understand all of them yet, will wait for a review -- unless Neil takes care of that before I do ;-) > The following already-existing macros are defined as aliases, such > that their semantics is unchanged (although scm_is_bool used to be a > function and is now a macro). > > scm_is_null --> scm_is_null_and_not_lisp_nil > scm_is_false --> scm_is_false_and_not_lisp_nil > scm_is_true --> scm_is_true_or_lisp_nil > scm_is_bool --> scm_is_bool_and_not_lisp_nil This part sounds right to me, based on the current semantics. > (I still believe that these should be changed to versions that handle > %nil properly, but await approval on that point, so these patches do > not make those changes) Yes, this also sounds right to me. > Also, if the preprocessor macro SCM_ENABLE_ELISP is not true (this > macro already existed and was used in lang.h), all overheads > associated with %nil handling are eliminated from the above macros. Excellent. Hacks like this are excellent :-) > vm-fixes.patch changes semantics, by fixing %nil handling in the > following instructions: br-if, br-if-not, br-if-null, br-if-not-null, > not, not-not, null?, and not-null? Sounds great in principle, though i have not looked at it > srfi-1-fixes.patch changes semantics, by fixing %nil handling in > several functions. Note that this patch (and several other large > forthcoming patches) will be unnecessary if the scm_is_false, > scm_is_true, and scm_is_null macros are changed to handle %nil as I > proposed. Hm. Perhaps we should decide first. > non-essential.patch is the last and least important. It doesn't > change any functionality or implementation. It changes two > occurrences of scm_is_bool and scm_is_null, in which %nil must *not* > be treated as a boolean or null, to use newly-added equivalent macros > which are explicit about how nil should be handled. These changes > will be needed if scm_is_null is changed as I proposed. It also adds > a few comments related to %nil handling. > > I've run "make check" on recent git master (c4b681fd) with these > patches applied, and everything seems to work. Sounds good too. > I haven't yet run any benchmarks, because I'm not sure how to best do > that. I doubt the changes will make any noticeable difference except > possibly in C code which does a lot of tests which include %nil. Yeah I don't expect too many differences either. Still, nice to clean up. Regards, Andy -- http://wingolog.org/
