declarations for with-html-output and with-html-output-to-string #9

Merged
merged 3 commits into from May 1, 2012

Conversation

Projects
None yet
2 participants
@alaa-alawi
Contributor

alaa-alawi commented Apr 27, 2012

as per the documentation of these macros, they do take declarations. Before the patch they will be placed incorrectly.
The patch try to put them in a more appropriate places. where it puts them under with-output-to-string and let for with-html-output-to-string and with-html-output respectively.

@hanshuebner

This comment has been minimized.

Show comment
Hide comment
@hanshuebner

hanshuebner May 1, 2012

There is a typo here (exctracted)

There is a typo here (exctracted)

This comment has been minimized.

Show comment
Hide comment
@alaa-alawi

alaa-alawi May 1, 2012

Owner

fixed. Thanks

Owner

alaa-alawi replied May 1, 2012

fixed. Thanks

@hanshuebner

This comment has been minimized.

Show comment
Hide comment
@hanshuebner

hanshuebner May 1, 2012

Please no cl:do in ediware.

Please no cl:do in ediware.

This comment has been minimized.

Show comment
Hide comment
@alaa-alawi

alaa-alawi May 1, 2012

Owner

I'd changed it to use LOOP. But a side question: any problems with DO macro?

Owner

alaa-alawi replied May 1, 2012

I'd changed it to use LOOP. But a side question: any problems with DO macro?

@hanshuebner

This comment has been minimized.

Show comment
Hide comment
@hanshuebner

hanshuebner May 1, 2012

I'd like to see this in the upstream version, with two corrections in util.lisp. Can you supply them?

I'd like to see this in the upstream version, with two corrections in util.lisp. Can you supply them?

This comment has been minimized.

Show comment
Hide comment
@hanshuebner

hanshuebner May 1, 2012

Adding a dependency on :alexandria and using alexandria:parse-body instead of extract-declarations would be acceptable.

Adding a dependency on :alexandria and using alexandria:parse-body instead of extract-declarations would be acceptable.

This comment has been minimized.

Show comment
Hide comment
@alaa-alawi

alaa-alawi May 1, 2012

Owner

I've made the changes and pushed them. I'd avoided adding :alexandria, since cl-who is still lean (no outside dependencies)

Owner

alaa-alawi replied May 1, 2012

I've made the changes and pushed them. I'd avoided adding :alexandria, since cl-who is still lean (no outside dependencies)

@hanshuebner

This comment has been minimized.

Show comment
Hide comment
@hanshuebner

hanshuebner May 1, 2012

There is nothing wrong with DO except that it is hard to read, but of course, your directly translated loop is not that much easier to read. Sorry. I have proposed to use Alexandria because it has a function that does precisely what you want to do, and in the age of Quicklisp, there is no reason to not use other libraries.

Here is a loop that I'd like:

(defun extract-declarations (forms)
(loop with declarations
for forms on forms
for form = (first forms)
while (eql (first form) 'cl:declare)
do (push form declarations)
finally (return (values (nreverse declarations) forms))))

There is nothing wrong with DO except that it is hard to read, but of course, your directly translated loop is not that much easier to read. Sorry. I have proposed to use Alexandria because it has a function that does precisely what you want to do, and in the age of Quicklisp, there is no reason to not use other libraries.

Here is a loop that I'd like:

(defun extract-declarations (forms)
(loop with declarations
for forms on forms
for form = (first forms)
while (eql (first form) 'cl:declare)
do (push form declarations)
finally (return (values (nreverse declarations) forms))))

This comment has been minimized.

Show comment
Hide comment
@alaa-alawi

alaa-alawi May 1, 2012

Owner

This works with the note that the last form in the following samples (although many are only for testing purposes - hypothetical) return as a second value (nil), rather than nil. which may case problem upon expansion while in macro.

(extract-declarations '((declare (special x)) (list 1 2 3 5)))
(extract-declarations '((declare (special x)) (declare (fixnum x)) (list 1 2 3 5)))
(extract-declarations '((declare (special x)) (list 1 2 3 5) (declare (fixnum x))))
(extract-declarations '((list 1 2 3 5) (declare (fixnum x))))
(extract-declarations '((declare (special x)) (list 1 2 3 5)))
(extract-declarations '((list 1 2 3 5)))
(extract-declarations '(()))

I'm fine with your implementation, even using Alexandria (although I do not support it). my goal is to get published declarations to work in cl-who macros.

one more side question: in my second variation using LOOP, which part was not clear? was it the termination part? the accumulation part? the stepping part? or all of them?

Kindly point to the one that works best with the long term goals of cl-who library. and I'll push them my fork.

Owner

alaa-alawi replied May 1, 2012

This works with the note that the last form in the following samples (although many are only for testing purposes - hypothetical) return as a second value (nil), rather than nil. which may case problem upon expansion while in macro.

(extract-declarations '((declare (special x)) (list 1 2 3 5)))
(extract-declarations '((declare (special x)) (declare (fixnum x)) (list 1 2 3 5)))
(extract-declarations '((declare (special x)) (list 1 2 3 5) (declare (fixnum x))))
(extract-declarations '((list 1 2 3 5) (declare (fixnum x))))
(extract-declarations '((declare (special x)) (list 1 2 3 5)))
(extract-declarations '((list 1 2 3 5)))
(extract-declarations '(()))

I'm fine with your implementation, even using Alexandria (although I do not support it). my goal is to get published declarations to work in cl-who macros.

one more side question: in my second variation using LOOP, which part was not clear? was it the termination part? the accumulation part? the stepping part? or all of them?

Kindly point to the one that works best with the long term goals of cl-who library. and I'll push them my fork.

This comment has been minimized.

Show comment
Hide comment
@hanshuebner

hanshuebner May 1, 2012

This comment has been minimized.

Show comment
Hide comment
@alaa-alawi

alaa-alawi May 1, 2012

Owner

I shall push your LOOP based implementation soon.

For the NIL thing. I meant that if any value of the the two values returned are nil, then this means that part (declarations or remaining forms) was not found.

Owner

alaa-alawi replied May 1, 2012

I shall push your LOOP based implementation soon.

For the NIL thing. I meant that if any value of the the two values returned are nil, then this means that part (declarations or remaining forms) was not found.

This comment has been minimized.

Show comment
Hide comment
@hanshuebner

hanshuebner May 1, 2012

changed the implementation of extract-declarations as per Hans reques…
…t - to be more readable/easier to reason about.

@hanshuebner hanshuebner merged commit 48516b6 into edicl:master May 1, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment