Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more ctype extension data structures #23

Merged
merged 13 commits into from
May 17, 2023

Conversation

charJe
Copy link
Contributor

@charJe charJe commented May 13, 2023

  • Add define-commutative-method and refactor existing methods to use it.

  • Add define-extended-type and extended-specifier-ctype for extending custom type specifiers to parse into ctypes.

  • Add extended types for list-of, array-of, and hash-table-of.

  • Add ctype/ext ASDF system.

cmember.lisp Show resolved Hide resolved
conjunction.lisp Outdated Show resolved Hide resolved
@@ -57,3 +57,15 @@
"cfunction" "packages"))
(:file "parse"
:depends-on ("generic-functions" "create" "classes" "config" "packages"))))

(asdf:defsystem :ctype/ext
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps in the future this system could consume the other bundled extensions. Alternatively perhaps you want them to be more granular than this.

disjunction.lisp Outdated Show resolved Hide resolved
(:documentation "Homogeneous array ctype."))

(defun carray-of (element-ctype &optional (dims '*) (upgraded-element-type '*) simplicity)
(if simplicity
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed something about not all implementations supporting complex vs simple arrays, but apparently they all do? Should I worry about this here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SICL will have only simple arrays, and in general I don't want to impose any more requirements than the standard does. The standard does allow having only simple arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

carray has disjoin/2 defined so that (disjoin (array integer) (array float)) becomes (array (or integer float)). I expect it to become just (or (array integer) (array float)).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with list-of, I think you're right and the code's wrong.

ext/tfun/tfun.lisp Outdated Show resolved Hide resolved
negation.lisp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@charJe charJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this review are only related to define-commutative-method.

cmember.lisp Show resolved Hide resolved
conjunction.lisp Outdated Show resolved Hide resolved
disjunction.lisp Outdated Show resolved Hide resolved
negation.lisp Outdated Show resolved Hide resolved
(deftype ,name ,lambda-list
,documentation
,@simple)
(setf (get ',name 'extended-type-parser)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use the symbol property list. I hope that's okay.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means the environment is ignored, but I have some thoughts on that I'll put elewhere

Copy link
Member

@Bike Bike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some concerns about the extended specifiers, since it's essentially a language extension, and I think it needs a bit more work. But as an experimental extension to this library I guess it's fine, provided that it doesn't interfere with using this library as an unextended typep/subtypep. I'm going to make a issue (#24) separate from this PR for how I think parsing ought to work.

cmember.lisp Outdated
(lambda (single-member)
(ctypep single-member ctype))
(cmember-members cmember))
(not (some (lambda (single-member)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use notany instead of not some

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I half blame git for this one.

(:documentation "Homogeneous array ctype."))

(defun carray-of (element-ctype &optional (dims '*) (upgraded-element-type '*) simplicity)
(if simplicity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SICL will have only simple arrays, and in general I don't want to impose any more requirements than the standard does. The standard does allow having only simple arrays.

parse.lisp Outdated
EXTENDED is a list of forms that return a ctype that completely represents the
custom type.

Both the SIMPILE and the EXTENDED forms share the parameters of LAMBDA-LIST.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"SIMPLE"


Both the SIMPILE and the EXTENDED forms share the parameters of LAMBDA-LIST.

LAMBDA-LIST is an ordinary lambda list that also allows &environment."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it not a deftype lambda list? More broadly, I think the right thing to do here is to have these work like macroexpanders - the expansion function takes the whole type specifier and the environment as arguments, and then parses the type specifier according to the non-env parts of the lambda list. But that will take work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it a deftype lambda list I will need to add &whole; I can use the same trick that I used for &environment. I will also need to make '* the default for &optional and &key. which will be more cumbersome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is some code for this in https://github.com/s-expressionists/ecclesia and/or SICL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I know that this would be work and I'll accept the PR without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deftype lambda lists also allow for destructuring parameters. I'm not sure how to do that cleanly.

(deftype ,name ,lambda-list
,documentation
,@simple)
(setf (get ',name 'extended-type-parser)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means the environment is ignored, but I have some thoughts on that I'll put elewhere

@@ -1,5 +1,10 @@
(in-package #:ctype)

(defmacro define-commutative-method (name arg1 arg2 &body body)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

define-commutative-method should look like defmethod - so the lambda list here should be (name (arg1 arg2) &body body). This confused me a couple times looking through the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you agree that it should still only be for methods taking 2 arguments?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. this is just a syntax thing

@Bike Bike merged commit 90af93d into s-expressionists:main May 17, 2023
@charJe charJe deleted the data-structures branch May 17, 2023 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants