RFC: Give MethodMatch its own type #36702
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Yes I definitely like this. |
vtjnash
reviewed
Jul 17, 2020
src/jltypes.c
Outdated
| @@ -2383,6 +2384,11 @@ void jl_init_types(void) JL_GC_DISABLED | |||
| jl_perm_symsvec(2, "typ", "fields"), | |||
| jl_svec2(jl_any_type, jl_array_any_type), 0, 0, 2); | |||
|
|
|||
| jl_method_match_type = jl_new_datatype(jl_symbol("MethodMatch"), core, jl_any_type, jl_emptysvec, | |||
| jl_perm_symsvec(4, "metharg", "methsp", "method", "fully_covers"), | |||
There was a problem hiding this comment.
Can we give these real words? (or at least the same mangling we do now):
signature (or specTypes), env (or sparam_vals), method, <: (or whatever this is)?
There was a problem hiding this comment.
This is what they were called in inlining. I agree we can pick better names. How about just specTypes, sparams, method, fully_covers? I don't like using <: as a field name.
There was a problem hiding this comment.
How about not mixing snake case and camel case? 😬
There was a problem hiding this comment.
spec_types then, though at some point we should also switch over MethodInstance then.
Keno
force-pushed
the
kf/methodmatch
branch
2 times, most recently
from
5dfd35e to
606cd91
Compare
|
Alright, this should be done now. |
Every time I see any code dealing with the svecs we get back from the method match code, I think that it shouldn't really be an svec: 1. It's always the same length 2. Basically every use of it typeasserts the field type 3. Every use of it needs the same magic numbers to access the fields. All put together this should just be a type. This updates all the uses accordingly, but adds fallback getindex/iterate defintions for external users that expect this to be a SimpleVector (in deprecated.jl - hopefully by 2.0 all external users will upgraded).
simeonschaub
pushed a commit
to simeonschaub/julia
that referenced
this pull request
Aug 11, 2020
Every time I see any code dealing with the svecs we get back from the method match code, I think that it shouldn't really be an svec: 1. It's always the same length 2. Basically every use of it typeasserts the field type 3. Every use of it needs the same magic numbers to access the fields. All put together this should just be a type. This updates all the uses accordingly, but adds fallback getindex/iterate defintions for external users that expect this to be a SimpleVector (in deprecated.jl - hopefully by 2.0 all external users will upgraded).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Every time I see any code dealing with the svecs we get back from
the method match code, I think that it shouldn't really be an svec:
All put together this should just be a type. This basically does that,
but there's a few things missing, so before I go finish it, I figured,
I'd make sure that there isn't some reason not to do this.
TODO:
MethodMatchto avoid breaking packages that use the internal APIDoesn't need extensive review in this state, since it isn't done,
just a quick yay/nay on the direction.