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

reflection: move signature union-splitting logic under the control of inference #22144

Merged
merged 2 commits into from
May 31, 2017

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented May 30, 2017

It just makes more sense and is more useful for inference to have control over the union-splitting.

edit: I split a function, so https://github.com/JuliaLang/julia/pull/22144/files?w=1 is a bit easier to read

@vtjnash vtjnash added the compiler:inference Type inference label May 30, 2017
@vtjnash vtjnash force-pushed the jn/method-union-splitting branch from 3489694 to 7529a15 Compare May 30, 2017 20:32
@@ -457,7 +457,6 @@ try
end
finally
@test pop!(LOAD_PATH) == path
rm(path, recursive=true)
Copy link
Contributor

Choose a reason for hiding this comment

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

the parent dirs get created. and what does this have to do with the rest of this pr?

Copy link
Member Author

Choose a reason for hiding this comment

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

This test is literally for the case "even though the path do no exist" (sic)

It doesn't, but it was tripping up my attempts at testing locally so it ended up on all my branches today.

Copy link
Contributor

Choose a reason for hiding this comment

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

only the first call, before mkpath gets run, and files get created under it. something else was happening in your case, open a separate issue instead of putting it into multiple unrelated pr's please

@vtjnash vtjnash force-pushed the jn/method-union-splitting branch from 7529a15 to 8ea4f16 Compare May 31, 2017 04:34
xapplicable = _methods_by_ftype(sig_n, sv.params.MAX_METHODS, sv.params.world, min_valid, max_valid)
xapplicable === false && return Any
append!(applicable, xapplicable)
end
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth to keep track of how many different methods the entries in applicable actually refer to and bail out if that is more than sv.params.MAX_METHODS, too?

Copy link
Member Author

Choose a reason for hiding this comment

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

probably, for now I'm just moving the existing logic rather than trying to improve on it much

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

@martinholters
Copy link
Member

👍 Makes the code clearer by moving things where they belong. (Also a good idea to break #21933 into smaller, more digestible pieces.)

@vtjnash vtjnash merged commit 88c21f4 into master May 31, 2017
@vtjnash vtjnash deleted the jn/method-union-splitting branch May 31, 2017 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants