-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
gf: support more dispatch on abstract types #31916
Conversation
I think not being able to define methods for |
ya' know the Community will be dancing while this becomes second nature 🌞 |
This removes the restriction on defining dispatch over user-defined abstract types. The "cannot add methods to an abstract type" error is now only applicable to a couple types (`Any`, `Function`, and functions), and instead now gives a "not implemented yet" message. fixes #14919 for 99% of cases
@nanosoldier |
This benchmark from #21760 goes from .034 sec to .042 sec on this branch:
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
@JeffBezanson I ran that code locally, and saw no performance difference. Probably just noise or a different random seed. |
else { | ||
tn = jl_new_typename_in((jl_sym_t*)name, module); | ||
if (super == jl_function_type || super == jl_builtin_type || jl_symbol_name(name)[0] == '#') { | ||
// Callable objects (including compiler-generated closures) get independent method tables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not catch the case of defining struct Foo <: Function
, which is probably fine, just want to check that that's the intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, that was my intention (it's even part of my examples at the top). Eventually the goal is probably just to merge all of the tables, but for now, I just merge the subset that I expect will be small.
//#else | ||
//#define write_ulong(s, x) (write_uint32((s), (x))) | ||
//#define read_ulong(s, x) (read_uint32((s), (x))) | ||
//#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete?
Running into a segmentation fault that bisect blames on this, JuliaGPU/CUDAnative.jl#407 |
What if we were to allow this, but only in cases where a method error would be thrown? Ie. make |
Awesome I've been waiting for this: https://github.com/caseykneale/ChemometricsTools.jl/blob/master/src/RegressionModels.jl How can developers maintain backward compatibility with previous julia versions with the implementation of this(once its streamlined)? Maybe the correct solution is to tell users to use the latest version of julia. |
This is a language feature that cannot be implemented in a package, so the only option of one needs this is to use a newer Julia version. |
…is is valid Julia syntax, see JuliaLang/julia#31916).
This removes the restriction on defining dispatch over user-defined abstract types. The "cannot add methods to an abstract type" error is now only applicable to a couple types (`Any`, `Function`, and functions), and instead now gives a "not implemented yet" message. fixes #14919 for 99% of cases
This removes the restriction on defining dispatch over user-defined abstract types. This fixes #14919 for 99% of cases.
The "cannot add methods to an abstract type" error is now only applicable to a couple types (
Any
,Function
, and functions), and instead now gives a "not implemented yet" message (that's perhaps not entirely right, since in several instances of this, I think they would be ambiguous with calling a Builtin function—and we don't need to ever enable that particular foot-gun).This PR actually hilariously similar to my branch #14919 (comment) from 3 years ago, but adds a few more error checks so that performance is unaffected.