Clean @pure issues in "dimensions.jl" and "static.jl"
#119
Conversation
Codecov Report
@@ Coverage Diff @@
## master #119 +/- ##
==========================================
+ Coverage 84.76% 85.85% +1.08%
==========================================
Files 8 8
Lines 1510 1527 +17
==========================================
+ Hits 1280 1311 +31
+ Misses 230 216 -14
Continue to review full report at Codecov.
|
Cleaned up code in src/dimensions.jl and test/dimensions.jl
There was a problem hiding this comment.
Looks good. My primary suggestion is to add Base.@aggressive_constprop, either using @static if VERSION >= v"1.7.0-DEV.421" (or whatever the specific version that added it is, if you want to spend the time to find out) to switch between two definitions, or maybe make one definition of each function annotated with @aggressive_constprop and have:
@static if VERSION >= v"1.7.0-DEV.421"
using Base: @aggressive_constprop
else
macro aggressive_constprop(ex)
ex
end
end| @@ -208,6 +208,10 @@ For example, if `A isa Base.Matrix`, `offsets(A) === (StaticInt(1), StaticInt(1) | |||
|
|
|||
| Is the function `f` whitelisted for `LoopVectorization.@avx`? | |||
|
|
|||
| ## static(x) | |||
There was a problem hiding this comment.
Are we at the point yet where we should just create a Static.jl and depend on it?
We have static Ints, Bools, and now Symbols.
Octavian also has staticfloats.
There was a problem hiding this comment.
I was thinking the same thing. There is already StaticNumbers.jl but I think there is a lot of utility in a very restricted set of static types. I think there has been a tendency with projects that deal with "staticness" to just solve everything with a new static type, which creates another problem.
There was a problem hiding this comment.
Where do think a package like Static.jl should go?
There was a problem hiding this comment.
I just created SciML/Static.jl and added you as a contributor.
I'm not sure about ArrayInterface's functionality vs StaticNumbers.jl, but ArrayInterface's has definitely been growing.
I should've added static ranges to the list of things we support here.
But I do agree on the idea of having a narrower scope / focusing on being a lightweight dependency that other packages that want such functionality (including of course ArrayInterface.jl) can cheaply depend upon.
|
Looks good. Merge when ready. |
This doesn't completely fix #115, but it does take care of the problems with
Symbol. I'll update that issue with what still needs to be worked on.Although this commit changes how
dimnamesworks and completely gets rid of a couple methods it shouldn't be breaking because none of these are actually part of the README and no packages should depend on them at this point.I also moved
staticto the README because I think it's worth making part of the public API. I think it' more convenient than typing out the entirety of a static types name.