scymtym / architecture.builder-protocol Goto Github PK

The arguments to ERROR in add-relations are incorrectly indented,
started from the second argument.

According to the documentation string of cardinality-case, the first element
of a clause is a "cardinality designator". A cardinality designator can be
(:map KEY-VAR), and then it says that KEY-VAR is bound to KEY in
some other cardinality designator shown as (:map KEY). But it does
not make much sense for a cardinality designator to bind a variable
for another cardinality designator. Perhaps the latter was supposed to be
a cardinality rather than a cardinality designator? But in that case, should
it not be (:map . KEY) rather than (:map KEY)?

I wouldn't consider it idiomatic to use members of LAMBDA-LIST-KEYWORDS for binding lexical variables - I was surprised to see it in the stack traces when Cleavir crashed on it at robert-strandh/SICL#132

I would add two things to the comment associated with make-keyword-arguments.

The first thing is that the list returned is initially (:key1 nil :key2 nil ...) so that the
first value of each list appears only after the first call to the destructive function.

The second thing is that, when a list of values is exhausted, then subsequent
calls to the destructive function will insert NIL, as opposed to (say) signaling
some error.

The LOOP body of class-initarg-and-relation-slots is not
correctly indented. I recommend the use of the contribution
slime-indentation to get it right.

It is not clear what the nature of VALUES is, and it is not
clear that the elements of the list are returned as multiple
values by the default method.

By starting an ASDF system definition file with (in-package #:asdf-user), you gain
two advantages. First, programming tools will be informed that this is the right
package to use in order to interpret the file contents. Second, if the ASDF file
is not in a place where ASDF can find it, the programmer can then use LOAD
to load it, without updating the places ASDF is looking. This last point is
especially useful when there are two versions of the system in the system, for
instance one in Quicklisp to be used for "production" and one that is used for
development or experiments.

The definition of relation-slot? looks very strange.
A minor issue is that the Scheme convention for
predicates is used. But the major strangeness is
that the function is presumably a predicate, but then
it returns the value of a call to slot-type->cardinality,
which presumably returns a cardinality which does
not look like a Boolean value. So this definition is
not understandable by itself and forces the reader to
understand the details of a different function. A
cardinality is usually a number, but this library also
defines a relation-cardinality, which is also not a
Boolean value.

I can not parse the entry for (:map . KEY) in the documentation string for
add-relations. It says that RIGHT is a sequence of nodes, but then
it mentions keys in the sequence, so there seem to be some keys in
that sequence as well as nodes.

By the way, the preposition to use for "associated" is "with", and not "to".

The use of previous-cell and previous-updater as Boolean values, has the
undesirable effect of handling the base cases after the "induction steps".
Since it is best to handle base cases first, i.e., in the "then" branch of the
IF, I recommend using (null previous-cell) and (null previous-updater) instead
and reversing the "then" and "else" branches. It would improve the code in
two ways. First, the base cases would be handled first, and second, it
would be clear that these variables are not Booleans, so that NIL is not
a false value, but a default value.

I might have written make-keyword-arguments like this:

(defun make-keyword-arguments (multi-keyword-arguments)
(loop for (keyword values) on multi-keyword-arguments by #'cddr
collect keyword into result
collect nil into result
collect values into values-list
finally (return (values result
(lambda ()
(loop for rest on (cdr result) by #'cddr
for values on values-list
do (setf (car rest) (pop (car values)))))))))
It has fewer lexical variables and its control structure is simpler.