[drakma-devel] Conditional (#+/#-) expressions in DRAKMA

[Joshua TAYLOR]

On Wed, Mar 4, 2009 at 3:22 AM, Edi Weitz <edi at agharta.de> wrote:
> On Wed, Mar 4, 2009 at 4:10 AM, Joshua TAYLOR <tayloj at cs.rpi.edu> wrote:
>
>> With the new release of DRAKMA I was looking over some of the source,
>> and came across code like
>>
>> (some-function ...#+:clisp #+:clisp :clisp-arg clisp-val)
>>
>> which I'd never seen before. I started to investigate, and a
>> discussion even started on c.l.l [1]. It turns out that not all Lisps
>> treat these constructions in the same way. Particularly, in the case
>> of #-feature #-feature, some process the second #-feature with
>> *READ-SUPPRESS* bound to T, so the behavior is like #-CL:NIL. At any
>> rate, it seems a bit safer and more portable not to nest the
>> conditionalized expressions. The only place I found these was in
>> request.lisp, and even there only in three places.
>>
>> I've attached a diff in which they are changed to the safer form. I've
>> also added leading colons to openmcl, since the rest of the source
>> seemed to use that style.
>
> Thanks for the patch.  I'm aware of the theoretical implications of
> this usage, but is it actually a problem in practice, i.e. does the
> code break for one of the supported Lisps?  And, without reading the
> c.l.l discussion, is this maybe because the Lisp in question is not
> following the standard correctly?

The /present/ code does /not/ break for any of the supported Lisps.
However, I do see conditionalizations on :openmcl, and it's
openMCL/ClozureCL that has this problem:

CL-USER> (lisp-implementation-version)
"Version 1.3-RC1-r11719M  (DarwinX8664)"
CL-USER> (lisp-implementation-type)
"Clozure Common Lisp"
CL-USER> (list #+:openmcl #+:openmcl 1 2 3 4)
(1 2 3 4)
CL-USER> (list #-:openmcl #-:openmcl 1 2 3 4)
(2 3 4) ; I bet you weren't expecting this ^_^

This issue doesn't arise directly in the current code, but will come
up if #-:openmcl #-:openmcl ever arises. This is just an attempt to
make that less likely to happen (since it would differ from the
surrounding style).

> I'd be willing to accept the patch although I like the current form
> more, but it'd be nice to know what we really lose or win.

As above, it's really not a significant issue, and you'd probably be
fine staying with the existing convention. The folks on c.l.l seem to
agree that the last form is permissible by the standard. (I did send a
message to the CCL people too, though, since there was a related bug,
and they're doing something different than what many people have come
to expect.)

> Also,
> could you please resend the patch as a unified diff and without tab
> characters?
>
>  http://weitz.de/patches.html

My apologies--I've even read that page before! I missed the `diff -u',
and I'd assumed that Emacs/Slime wasn't putting in literal #\Tab
characters (now something of which I'll have to be aware in the
future). Updated diff attached. Again, the cost of non-adoption is
very low, and benefits are only preventing /possible/ /future/ bugs.

//JT
-------------- next part --------------
--- request.lisp.old	2009-03-03 21:54:27.000000000 -0500
+++ request.lisp	2009-03-04 08:17:51.000000000 -0500
@@ -157,8 +157,8 @@
                      (when textp
                        (setf result
                              (octets-to-string result :external-format (flexi-stream-external-format stream))
-                             #+:clisp #+:clisp
-                             (flexi-stream-element-type stream) element-type))
+                             #+:clisp (flexi-stream-element-type stream)
+                             #+:clisp element-type))
                      result))
                   ((or chunkedp must-close)
                    ;; no content length, read until EOF (or end of chunking)
@@ -204,7 +204,7 @@
                               #+:lispworks (read-timeout 20)
                               #+(and :lispworks (not :lw-does-not-have-write-timeout))
                               (write-timeout 20 write-timeout-provided-p)
-                              #+openmcl
+                              #+:openmcl
                               deadline)
   "Sends an HTTP request to a web server and returns its reply.  URI
 is where the request is sent to, and it is either a string denoting a
@@ -446,17 +446,18 @@
                                                           :timeout connection-timeout
                                                           :read-timeout read-timeout
                                                           #-:lw-does-not-have-write-timeout
+                                                          :write-timeout
                                                           #-:lw-does-not-have-write-timeout
-                                                          :write-timeout write-timeout
+                                                          write-timeout
                                                           :errorp t)
                                     #-:lispworks
                                     (usocket:socket-stream
                                      (usocket:socket-connect host port
                                                              :element-type 'octet
-                                                             #+openmcl #+openmcl
-                                                             :deadline deadline
+                                                             #+:openmcl :deadline
+                                                             #+:openmcl deadline
                                                              :nodelay t))))
-              #+openmcl
+              #+:openmcl
               (when deadline
                 ;; it is correct to set the deadline here even though
                 ;; it may have been initialized by SOCKET-CONNECT