current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite
1

I've been programming Clojure for a little while and recently started learning Common Lisp. One of my favorite things about Clojure is the threading operator ->, which greatly simplifies long chains of nested function calls. Naturally I wanted to have this in Common Lisp.

I found an implementation here:

(defmacro -> (x &rest args)
  (destructuring-bind (form &rest more)
      args
    (cond
      (more `(-> (-> ,x ,form) ,@more))
      ((and (consp form)
            (or (eq (car form) 'lambda)
                (eq (car form) 'function)))
       `(funcall ,form ,x))
      ((consp form) `(,(car form) ,x ,@(cdr form)))
      (form `(,form ,x))
      (t x))))

This uses a recursive macro expansion; I've read that it's better to use iteration over recursion in CL when you can, so I wrote my own version:

(defmacro -> (x &rest forms)
  (labels ((expand-form (x form)
             (if (consp form)
                 (if (or (eq (car form) 'lambda)
                         (eq (car form) 'function))
                     `(funcall ,form ,x)
                     `(,(car form) ,x ,@(cdr form)))
                 `(,form ,x))))
    (do ((forms forms (cdr forms))
         (x x (expand-form x (car forms))))
        ((not forms) x))))

I'm relatively new to Lisp so I don't know how to judge which way is better. Any comments, suggestions?

EDIT: I knew something was fishy! You don't have to use any looping constructs at all, a plain old reduce will cut it:

(defmacro -> (x &rest forms)
  (flet ((expand-form (x form)
           (cond
             ((atom form)
              `(,form ,x))
             ((member (car form) '(lambda function))
              `(funcall ,form ,x))
             (t
              `(,(car form) ,x ,@(cdr form))))))
    (reduce #'expand-form forms :initial-value x)))
share|improve this question
up vote 2 down vote accepted

I wouldn't be so focused on iteration vs. recursion; use what is necessary and convenient. For macros in general you should care about clarity of the macro and the generated code, performance of the macro code itself comes way last in general.

Now to add only two things to the discussion, using DO is not particularly common because in most cases there are better options; personally I always have to look up the meaning of all the clauses of DO, which is why I am not a fan of it. The other thing is to use COND to be a bit more concise if your IF clauses allow for it.

Since you now already have a good version of the first solution, the following would be a way to do those things for the iterative solution:

(defmacro -> (x &rest forms)
  (flet ((expand-form (x form)
           (cond
             ((atom form)
              `(,form ,x))
             ((member (car form) '(lambda function))
              `(funcall ,form ,x))
             (T `(,(car form) ,x ,@(cdr form))))))
    (loop
      for form in forms
      for y = (expand-form x form)
        then (expand-form y form)
      finally (return y))))

Note that I've switched (not (consp x)) to (atom x); now the LOOP is less concise then the DO, but I'd argue that it's more obvious what is happening with it, YMMV.

share|improve this answer
    
Thanks for the suggestions! I like the way you've rewritten the macro. I'm not favoring iteration over recursion for performance; I find it easier to understand this way. Also, I get the generated code with one macroexpand rather than with a code walker. – korrok Jan 11 '15 at 22:47
1  
Why use 'sub on forms' and later 'car sub'? Why not just use 'sub in forms'? – Rainer Joswig Jan 11 '15 at 23:44
    
... yes, I don't know why I wrote that, thanks @RainerJoswig. – ferada Jan 12 '15 at 10:45
1  
@korrok yes, that's basically why I often prefer the iterative version as well. If you didn't already knew, there is also macroexpand-all, not a standard function, but nevertheless used to completely expand an expression. – ferada Jan 12 '15 at 10:49
up vote 2 down vote

I think the first version is slightly more elegant and looks okay to me.

In the second version, I would replace LABELS with FLET. LABELS would indicate some kind of recursion.

Generally I would also think if the nested expansion is something I would want, vs. an expansion into a LET* form.

I would also replace CAR and CDR with FIRST and REST.

The DESTRUCTURING-BIND is also not necessary:

(defmacro -> (x form &rest more)
  (cond (more
         `(-> (-> ,x ,form) ,@more))
        ((and (consp form)
              (member (first form) '(lambda function)))
         `(funcall ,form ,x))
        ((consp form)
         `(,(first form) ,x ,@(rest form)))
        (form
         `(,form ,x))
        (t x)))
share|improve this answer
up vote 1 down vote

I wouldn't use labels here. To me, the use of labels or similar definition inside a definition is justified if I need to reuse the same function inside another function, or if it will in some sense make it more clear.

So, here's, for the sake of diversity, a version that uses iterate package.

(defmacro -> (x &rest forms)
  (iter
    (for form :in forms)
    (for y :=
        (case (car (when (consp form) form))
          ((lambda function) `(funcall ,form ,x))
          ('nil `(,form ,x))
          (otherwise `(,(car form) ,x ,@(cdr form)))))
    (setf x y)
    (finally (return y))))
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.