Int -> UInt8#41
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41 +/- ##
==========================================
+ Coverage 75.36% 76.07% +0.70%
==========================================
Files 2 2
Lines 203 209 +6
==========================================
+ Hits 153 159 +6
Misses 50 50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Defaulted to |
ReubenJ
left a comment
ReubenJ
left a comment
There was a problem hiding this comment.
Couple of questions, small changes. I'm also fine with the dynamic type parameter for a rulenode if you like.
| abstract type AbstractRuleNode end | ||
|
|
||
| # Interface to AbstractTrees.jl | ||
| # Integererface to AbstractTrees.jl |
|
|
||
| # Default constructor | ||
| function RuleNode(ind::Integer, _val::Any, children::Vector{<:AbstractRuleNode}=AbstractRuleNode[]) | ||
| new{smallest_Int_type(ind)}(ind, _val, children) |
There was a problem hiding this comment.
In favor of just defaulting to UInt8 here 👍
| @assert ex.head==:curly "Input to the @rulenode macro should be in the form of: 1{2,3}" | ||
| :(RuleNode( | ||
| # Interpolate the _value_ of the first rule (1 from 1{2,3}) | ||
| # Integererpolate the _value_ of the first rule (1 from 1{2,3}) |
There was a problem hiding this comment.
| # Integererpolate the _value_ of the first rule (1 from 1{2,3}) | |
| # Interpolate the _value_ of the first rule (1 from 1{2,3}) |
|
|
||
|
|
||
| """ | ||
| smallest_Int_type(n::Integer) | ||
|
|
||
| Finds the smallest type for the given integer and returns it. | ||
| """ | ||
| function smallest_Int_type(n::Integer) | ||
| for T in (UInt8, UInt16) | ||
| if typemin(T) ≤ n ≤ typemax(T) | ||
| return T | ||
| end | ||
| end | ||
| return Int | ||
| end No newline at end of file |
There was a problem hiding this comment.
| """ | |
| smallest_Int_type(n::Integer) | |
| Finds the smallest type for the given integer and returns it. | |
| """ | |
| function smallest_Int_type(n::Integer) | |
| for T in (UInt8, UInt16) | |
| if typemin(T) ≤ n ≤ typemax(T) | |
| return T | |
| end | |
| end | |
| return Int | |
| end |
| Evaluate immediately functionality is not yet supported by most of Herb.jl. | ||
| """ | ||
| RuleNode(ind::Int, _val::Any) = RuleNode(ind, _val, AbstractRuleNode[]) | ||
| # RuleNode(ind::Integer, _val::Any) = RuleNode(ind, _val, AbstractRuleNode[]) |
|
|
||
| # Default constructor | ||
| function RuleNode(ind::Integer, _val::Any, children::Vector{<:AbstractRuleNode}=AbstractRuleNode[]) | ||
| new{UInt8}(ind, _val, children) |
There was a problem hiding this comment.
I'd also be fine with using smallest_Int_type now that it's fast.
|
Also, just to note: this will have to be a breaking release ( To see an example, run |
| mutable struct RuleNode{T<:Integer} <: AbstractRuleNode | ||
| ind::T | ||
| _val::Any | ||
| children::Vector{AbstractRuleNode} |
There was a problem hiding this comment.
Wondering if we should switch this to AbstractVector while we're at it. This opens us up to using StaticArrays.jl' SVector or MVector. Probably some performance to squeeze out there.
3 participants
RuleNodeis now parametric and dynamically decides for the smallest typeThe default type for everything integer is not the concrete type
Int(which defaults toInt64on 64-bit machines) but now the abstract typeInteger(for whichUInt8 <: IntegerandInt <: Integerholds)I added
smallest_Int_type,which iterates over possible types to return the smallest.Why this is still a draft:
We have to decide what types we want to allow here—the more types we iterate over in
smallest_Int_type,the slower the enumeration potentially. I will test this as soon as possible.