Skip to content

remove subtypes search for concrete types #19964

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

Merged
merged 3 commits into from
Jan 30, 2017
Merged

remove subtypes search for concrete types #19964

merged 3 commits into from
Jan 30, 2017

Conversation

ZacLN
Copy link
Contributor

@ZacLN ZacLN commented Jan 10, 2017

This prevents an unnecessary/expensive call to _subtypes when subtypes is called on a concrete type.

This prevents an unnecessary/expensive call to `_subtypes` when `subtypes` is called on a concrete type.
@kshyatt kshyatt added the types and dispatch Types, subtyping and method dispatch label Jan 10, 2017
@kshyatt kshyatt requested a review from JeffBezanson January 10, 2017 17:03
@@ -301,7 +301,7 @@ function _subtypes(m::Module, x::DataType, sts=Set{DataType}(), visited=Set{Modu
end
return sts
end
subtypes(m::Module, x::DataType) = sort(collect(_subtypes(m, x)), by=string)
subtypes(m::Module, x::DataType) = x.abstract ? sort(collect(_subtypes(m, x)), by=string) : DataType[]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can use the opportunity to also improve this by changing it to sort!(collect(...))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, good point

@davidanthoff
Copy link
Contributor

Bump, is there any chance this could go into 0.6? We kind of need it for our LanguageServer.jl project, which is the VS Code julia extension.

@JeffBezanson, it looks like you might have to review this?

@tkelman tkelman added the needs tests Unit tests are required for this change label Jan 26, 2017
@KristofferC
Copy link
Member

Perhaps add a few tests for subtype to concrete types and this should be good in my opinion.

@ZacLN
Copy link
Contributor Author

ZacLN commented Jan 26, 2017

Do you mean a simple:

# PR19964
@test isempty(subtypes(Float64))```
?

@KristofferC
Copy link
Member

Yeah, just to hit that code path.

@ZacLN
Copy link
Contributor Author

ZacLN commented Jan 26, 2017

Cool, done

@tkelman tkelman removed the needs tests Unit tests are required for this change label Jan 26, 2017
@KristofferC
Copy link
Member

Can this be merged?

@KristofferC
Copy link
Member

I plan to merge this in a day or so unless I hear anything else.

@pabloferz
Copy link
Contributor

@yuyichao is working on some changes to subtypes over #20307, so maybe it's worth to ask for his opinion here?

@yuyichao
Copy link
Contributor

LGTM. This will conflict with #20307 but it should be easy to resolve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants