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

Use Compat.@constprop and Compat.@inline #205

Merged
merged 1 commit into from
Sep 16, 2021
Merged

Use Compat.@constprop and Compat.@inline #205

merged 1 commit into from
Sep 16, 2021

Conversation

timholy
Copy link
Member

@timholy timholy commented Sep 11, 2021

These are changing names/gaining functionality.
Compat allows one to easily support multiple Julia versions.

@timholy
Copy link
Member Author

timholy commented Sep 11, 2021

CC @aviatesk

@codecov
Copy link

codecov bot commented Sep 11, 2021

Codecov Report

Merging #205 (b5bf362) into master (84a3bce) will increase coverage by 1.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #205      +/-   ##
==========================================
+ Coverage   84.92%   86.20%   +1.27%     
==========================================
  Files          11       11              
  Lines        1665     1812     +147     
==========================================
+ Hits         1414     1562     +148     
+ Misses        251      250       -1     
Impacted Files Coverage Δ
src/ArrayInterface.jl 85.63% <ø> (+0.68%) ⬆️
src/dimensions.jl 97.10% <100.00%> (+0.15%) ⬆️
src/indexing.jl 77.98% <100.00%> (+3.08%) ⬆️
src/broadcast.jl 100.00% <0.00%> (ø)
src/array_index.jl 98.18% <0.00%> (+0.04%) ⬆️
src/size.jl 84.09% <0.00%> (+0.36%) ⬆️
src/ranges.jl 93.84% <0.00%> (+0.56%) ⬆️
src/stridelayout.jl 89.27% <0.00%> (+1.44%) ⬆️
... and 2 more

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 84a3bce...b5bf362. Read the comment docs.

@timholy
Copy link
Member Author

timholy commented Sep 11, 2021

Ah, I thought I was editing my own previous change, now I see this essentially happened in #204. This can be closed unless you want the @inline (which fixes a depwarn).

Copy link

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

yeah, I still prefer this PR because the Compat version does additional argument validation etc., so I think it's reasonable to merge it if we're okay to introduce new dependency on Compat.jl.

@Tokazama
Copy link
Member

I don't have a strong opinion on this one, but if it's a better band-aid it might be worth further discussion. Is the the main concern here the extra Compat.jl dependency? If so, could we just negotiate this concern by agreeing to drop Compat.jl with the next LTS? I'm assuming it will come around in the next yearish.

@timholy
Copy link
Member Author

timholy commented Sep 16, 2021

Yes, if this package will drop old Julia releases you can handle things that way. Compat's size as a dependency also decreases for people running newer Julia versions:

tim@diva:~/.julia/compiled$ ls -l v1.0/Compat/*.ji
-rw------- 1 tim holy 226711 Aug 31 04:33 v1.0/Compat/GSFWK.ji
tim@diva:~/.julia/compiled$ ls -l v1.8/Compat/*.ji
-rw-r--r-- 1 tim holy 69209 Aug 28 19:36 v1.8/Compat/GSFWK_eYLmO.ji
-rw-r--r-- 1 tim holy 69209 Sep  2 03:49 v1.8/Compat/GSFWK_FJfGL.ji
-rw-r--r-- 1 tim holy 70749 Sep 10 06:59 v1.8/Compat/GSFWK_mdmZ9.ji
-rw-r--r-- 1 tim holy 48135 Sep  1 05:09 v1.8/Compat/GSFWK_Ol4t2.ji
-rw-r--r-- 1 tim holy 72747 Sep 15 09:33 v1.8/Compat/GSFWK_V8kJc.ji
-rw-r--r-- 1 tim holy 70701 Sep 10 06:51 v1.8/Compat/GSFWK_wxM3A.ji
-rw-r--r-- 1 tim holy 70010 Sep 14 02:53 v1.8/Compat/GSFWK_yRJjT.ji
-rw-r--r-- 1 tim holy 69194 Aug 27 03:46 v1.8/Compat/GSFWK_z7wL7.ji
-rw-r--r-- 1 tim holy 69209 Aug 28 09:36 v1.8/Compat/GSFWK_ZcdGt.ji

So about 1/3. Most of that is probably defining @compat (which you then basically don't use).

@chriselrod
Copy link
Collaborator

Compat may be ubiquitous enough already that most people are already pulling it in anyway.

@Tokazama
Copy link
Member

Sounds like everyone is fine with this as a dep. We just need a patch bump.

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.

4 participants