Skip to content

Fix subclassp for T class - #624

Closed
hemml wants to merge 1 commit into
jscl-project:masterfrom
hemml:master
Closed

hemml wants to merge 1 commit into
jscl-project:masterfrom
hemml:master

Conversation

@hemml

@hemml hemml commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

The class T has no superclassess, so internal CLOS function SUBCLASSP, which is involved in appropriate method computation, returns NIL if both arguments are T:

CL-USER> (defclass c1 () ())

#<STANDARD-CLASS C1>
CL-USER> (jscl::subclassp (find-class 'c1) (find-class 'c1))
T
CL-USER> (jscl::subclassp (find-class T) (find-class T))
NIL

@hemml

hemml commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Can anybody merge this PR? It blocks me from updating OMGlib to the latest JSCL. Some CLOS methods cannot be called if they have mixed typed and untyped arguments: T sometimes not treated as a subclass of T.

@kchanqvq

Copy link
Copy Markdown
Member

Let me take a look. I think it's probably better to fix the class precedence list of T.

@hemml

hemml commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Anyway, this fix will speedup check for the most common cases. It will be great to fix also the class precedence, of course.

@kchanqvq

Copy link
Copy Markdown
Member

It's good to optimize performance, and in that case is eq better than equal? (Also let's state the commit message as performance improvement)

@hemml

hemml commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

I'm not a professional Lisp programmer (or even a programmer in general), so I'm just following my tutorial recommendation to use equal for safety) Yes, eq will be faster there.

@davazp

davazp commented Jul 28, 2026

Copy link
Copy Markdown
Member

Sorry I missed this PR! I saw it but I thought I merged it.

Let me known if you want to change equal to eq or leave and I’ll merge it.

@kchanqvq

Copy link
Copy Markdown
Member

The bug is already fixed in #625 btw, so this is just optimization.

@hemml hemml closed this by deleting the head repository Aug 23, 2026
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.

3 participants