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

WIP Remove dependency on inference #7

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

davidanthoff
Copy link
Member

No description provided.

@davidanthoff davidanthoff changed the title Remove map dependency on inference Remove dependency on inference Jan 11, 2018
@davidanthoff davidanthoff changed the title Remove dependency on inference WIP Remove dependency on inference Jan 11, 2018
@codecov-io
Copy link

codecov-io commented Jan 12, 2018

Codecov Report

Merging #7 into master will increase coverage by 0.82%.
The diff coverage is 98.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #7      +/-   ##
==========================================
+ Coverage   81.69%   82.51%   +0.82%     
==========================================
  Files          18       18              
  Lines         508      549      +41     
==========================================
+ Hits          415      453      +38     
- Misses         93       96       +3
Impacted Files Coverage Δ
src/source_iterable.jl 100% <100%> (ø) ⬆️
src/enumerable/enumerable_take.jl 93.33% <100%> (-6.67%) ⬇️
src/enumerable/enumerable_map.jl 100% <100%> (ø) ⬆️
src/enumerable/enumerable_drop.jl 93.33% <100%> (-6.67%) ⬇️
src/enumerable/enumerable_filter.jl 98.03% <97.05%> (-1.97%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ea901b...da6147a. Read the comment docs.


function _map(source::Enumerable, f::Function, f_expr::Expr, ::Base.HasEltype)
TS = eltype(source)
T = Base._return_type(f, Tuple{TS,})
Copy link
Contributor

Choose a reason for hiding this comment

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

This still relying on inference though? I think to really avoid Inference, we should mirror the implementation of collect on Generators and how the new Broadcast will work.

Copy link
Member Author

Choose a reason for hiding this comment

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

The strategy here is to use the information from inference if it gives us a concrete type, otherwise not use that information. I think that is the same strategy that Broadcast is using now? As far as I understand it, that would be an ok use of inference.

The question though is whether it actually buys us anything, or whether we could just never use it. At least for a intermediate period it would help with moving things over: it will take a while to change all the sinks to accept iterators that have the EltypeUnknown() trait, and until that is done this implementation here would still return things with HasEltype() in all cases where it does so today.

Choose a reason for hiding this comment

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

The strategy here is to use the information from inference if it gives us a concrete type, otherwise not use that information. I think that is the same strategy that Broadcast is using now? As far as I understand it, that would be an ok use of inference.

FWIW, I don't think the question is whether the inferred type is concrete or not. It's rather that the user-visible behavior should be the same whether or not inference gives you a precise type or Any.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants