[teepeedee2-devel] More TPD2 patches.

[Vladimir Sedach]

> To emphasise again, I am not going to apply patches which just change
> the names of variables, expand out the my stuff to make it longer and
> change the indentation, etc.

Ok, I'll stop sending those.

>> Subject: [PATCH] Refactored sendbuf.lisp to make the code easier to
>> read.
>
> For whom? ;-)

Anybody who doesn't know what my-defun etc. does.

>> +
>> +(defvar request-remote-ip*)
>> +(defvar request-remote-port*) ;; TODO
>> +(defvar request-headers*) ;; TODO
>> +(defvar request-method*)
>> +(defvar request-uri*)
>> +(defvar request-path*)
>> +(defvar request-raw-post-data*) ;; (file upload) TODO
>> +(defvar request-parameters*)
>
> Why do these things have only one earmuff?

That's a convention I borrowed from Hunchentoot 1.0.

> I kind of like this idea, and I guess I will test it out on my benchmark
> game to see if it is feasible. Have you done any benchmarks yourself?

Nope, still haven't gotten my code to run. However, I'm not sure this
(dynamically scoped variables) actually works. Is there any point
during the request dispatching where it can happen that the current
continuation is saved and the thread begins servicing another request?
If that's so then obviously this approach doesn't work.

I actually redid the patch to pass a struct containing all these
fields as an (anaphoric) parameter to the lambda that defpage
generates, which would work in the above scenario, however there's
still a bug there I haven't tracked down.

> This definitely will not fly. Definitely do not depend on .webapp in
> .http! Better to move the constant.

Yes, but I wanted to leave that for a separate patch.

>> -(defun default-http-error-page (dispatcher path params)
>> -  (declare (ignore params path dispatcher))
>> +(defun default-http-error-page ()
>>    (with-sendbuf ()
>>      "<h1>I made a mistake. Sorry for the inconvenience.</h1>"))
>
> I want the path and parameters to be passed to this function.

*If* the dynamic vars work, then you can get at them via request-path*
and request-parameters*. Otherwise, yeah, this is going to need the
request details as an argument.

>> +                                          ("x-forwarded-for" ;; FIXME
>> +                                           (setf request-remote-ip*
>>                                                   (match-x-forwarded-for value)))))))
>
> Why the FIXME?

I'm not sure having x-forwarded-for clobber the request-remote-ip
value is the right thing to do.

>> diff --git a/src/lib/macros.lisp b/src/lib/macros.lisp
>> index 35b0c76..e554106 100644
>> --- a/src/lib/macros.lisp
>> +++ b/src/lib/macros.lisp
>> @@ -178,16 +178,3 @@
>>  (defmacro let-current-values (vars &body body)
>>    `(let ,(loop for v in vars collect `(,v ,v))
>>       , at body))
>> -
>> -
>> -(defmacro with-preserve-specials (specials &body body)
>> -  (let ((tmps (mapcar (lambda(x)(gensym (symbol-name x))) specials)))
>> -    `(let ,(loop for s in specials
>> -              for m in tmps
>> -              collect `(,m (when (boundp ',s),s)))
>> -       (macrolet ((with-specials-restored (&body body)
>> -               `(let ,',(loop for s in specials
>> -                              for m in tmps
>> -                              collect `(,s ,m))
>> -                  , at body)))
>> -      , at body))))
>
> Why the hell are you deleting these macros?

It's much easier to understand (and just as easy to write) that code
just using LET.

>> -(defconstant-string +channel-page-name+ "/*channel*")
>> -
>> -(defconstant-string +action-page-name+ "/*action*")
>> -
> Why are you deleting these?

What's the difference between typing +action-page-name+ and
"/*action*"? The URI is what needs to be the same. Having an alias for
it just makes the code harder to grep.

Vladimir