From: Douglas K. <do...@go...> - 2015-06-12 14:20:41
|
Thank you for the thorough review. > I have not thought this through very deeply, but I don't think "up front" checking is needed. Removal of the up-front checks would be a regression. I suspect users have come to expect a friendlier message than "foo is not a list", without the context of "in a destructuring bind". Now granted, it's possibly because no implementation but SBCL took type declarations seriously enough to implement (destructuring-bind (a b c) X ...) as (let ((a (pop (the cons x))) (b (pop (the cons x))) ... ) and have conforming behavior in high safety. Thus it has become something of a tradition to perform the up-front checks. As mentioned, ITA deletes all the pre-checks and ordinary checks in unsafe code. > ... DEFTRANSFORMs I saw that, I find that to be somewhat over-designed. The use-case for a macro is nonexistent, since its input is just FORM. In the latest patch, in safe code, I've put (truly-the list) around each element extraction except when any user code has intervened into the expansion by a nontrivial defaulting form, so there's usually no redundant type-check after the pre-check. It's rare/unusual that the validation happens in a function call and that we trust that function implicitly, but it's not unheard-of. (And if threads/interrupts mutate data that have already been type-checked - that has no bearing on ds-bind. It's a general issue to break an assertion that the compiler has already validated. ) > * Could DESTRUCTURING-BIND itself be defined Too early. 'destructuring-bind' uses defmacro-mundanely which is in 'defmacro' which uses the "guts" of the expander without actually using destructuring bind. Doing it the other way requires moving a lot of things in 'defmacro'. > * Could the various :accept #.(…) arguments to PARSE-LAMBDA-LIST become global variables .. I'd prefer either to confine each mask to exactly one place it is needed, or have make-keyword-mask recognize additional symbols like 'DEFTYPE as a special case. They're all single-use constants except for 'destructuring-bind' as a kind which is specially recognized mainly out of convenience. Do you think we should imbue the mask-making function with knowledge of all possible modes of :accept? That did cross my mind initially. I guess, is the main concern to get rid of ugly #. ? I can re-check why it didn't work as a macro, but I remember I had issues with it. > Rename BUILD-LAMBDA-LIST to UNPARSE-LAMBDA-LIST? sgtm. There's a difference though: parse-DS-lambda-list parses into an abstract (non-list, but single encapsulated entity) representation, and can unparse from that. Contrastingly, BUILD- (non-destructuring) means "construct from pieces", whether it had been parsed or is being generated from thin air. As such, I'd probably call it MAKE- or CONSTRUCT- since it's not actually the unparser for a "thing". Wdyt? > Since some behavior changes, unit tests changes seem necessary This patch went out of its way not to break anything! (Despite my shock that we special-cased the outermost list not to signal the general destructuring-mismatch condition.) > While at it, we should test and maybe document behavior w.r.t [1]. CLISP is wrong, no question. CLHS is sane about this one - the default expression defaults for that piece of the pattern, but CLISP accidentally takes CAR/CDR of NIL. Change the test to (destructuring-bind (&optional ((foo bar) '(a . b))) nil (values foo bar)) and you'll see CLISP choke. > The patch doesn't touch SB-C::COMPILER-DESTRUCTURING-BIND. Acknowledged. I have to flesh it out a tiny bit more - I need to totally eradicate parse-defmacro as well. Next: > Rename RECONSTITUTE-DS-LAMBDA-LIST to > DS-BIND-EMIT-CHECK -> EMIT-DS-BIND-CHECK > EXPAND-DS-BIND could use a short comment explaining what Done > This is probably pedantic, but in ... I've rewritten it. I'll take another look > In WITH-MIN/MAX and CHECK-DS-LIST*, why is LIMITS a cons Idea stolen from another implementation. I'll make it pass two args. > I think WITH-ERR-CONTEXT should accept MARKER-VAR and ... Done > Could DS-GETF return a second value or accept a default Can't accept a default any reasonable semantics, the point is to avoid evaluating the defaulting form and avoid mv handling as well. In fact the convention is nearly the same as for compiler-generated lambdas - Stas made them use 0 for absence of a key. I'll add a comment. |