Skip to content

Conversation

@Tokazama
Copy link
Collaborator

There shouldn't be any changes to code inference. This does more of what #13 does but focuses on methods where one argument is static and one is dynamic, making it unnecessary to specialize on the static type.

@chriselrod, if this doesn't affect performance or constant propagation for your code libraries then it should be good to go.

@codecov
Copy link

codecov bot commented Sep 25, 2021

Codecov Report

Merging #19 (471e6bb) into master (baa9ee4) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
+ Coverage   98.33%   98.36%   +0.03%     
==========================================
  Files           7        7              
  Lines         421      429       +8     
==========================================
+ Hits          414      422       +8     
  Misses          7        7              
Impacted Files Coverage Δ
src/Static.jl 97.77% <100.00%> (ø)
src/bool.jl 100.00% <100.00%> (ø)
src/float.jl 98.64% <100.00%> (ø)
src/int.jl 100.00% <100.00%> (ø)
src/symbol.jl 100.00% <100.00%> (ø)
src/tuples.jl 100.00% <0.00%> (ø)
src/ndindex.jl 93.50% <0.00%> (+0.45%) ⬆️

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 baa9ee4...471e6bb. Read the comment docs.

@chriselrod
Copy link
Collaborator

chriselrod commented Sep 25, 2021

I'd have to run tests and benchmarks, but it looks like these are covered by @inferred tests here.

@Tokazama
Copy link
Collaborator Author

Tokazama commented Sep 26, 2021

I'd have to run tests and benchmarks, but it looks like these are covered by @inferred tests here.

I'm fine with whatever you're comfortable with. I don't want to give you unnecessary work but I also don't want to mess up any of your code.

I think the most likely problem would be slow downs due to using @inbounds(getfield(x, :parameters)[1]) to avoid specializing on dynamic results. I don't think any static computations should be affected.

Copy link
Collaborator

@chriselrod chriselrod left a comment

Choose a reason for hiding this comment

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

I don't really know anything about what @nospecialize does, how it is implemented, or what guarantees it'll have in the future, so I'm not really qualified to review.

@Tokazama
Copy link
Collaborator Author

Tokazama commented May 6, 2022

All of this was implement in #55

@Tokazama Tokazama closed this May 6, 2022
@chriselrod chriselrod deleted the nospecialize branch May 6, 2022 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants