-
Notifications
You must be signed in to change notification settings - Fork 0
Description
I'm annoyed by how common the pattern WrapperType(ContainerType, ContainerElementType) is in phase (views use it, and so does ReadonlyWrapper). In theory there's no need to store the element type, all that you need to know is the container type and the compiler should be able to infer the rest. However, I've thought about it a lot, and I don't think there's any way to do that - you need to know the element type in order to do things like include Indexable(T) or include MultiIndexable(T), which are quite crucial.
This isn't really a technical issue, but it's such an eyesore, especially on the end user, to see ReadonlyWrapper(Array(Int32), Int32), or worse, Phase::ProcView(NArray(Int32), Int32, Char) in their compiler errors.
Because of this, I explored the following 'solution' for ReadonlyWrapper:
module Phase
# A wrapper around an `Array` that allows reading but not writing (but the `Array`'s owner can mutate it).
# This is used to prevent the user from modifying the iterating coordinate in
# methods like `each_with_coord` without the performance penalties of cloning
# or double-buffering.
class ReadonlyArray(T)
include Indexable(T)
@src : Array(T)
delegate size, unsafe_fetch, inspect, to_s, to: @src
def initialize(@src : Array(T))
end
def unsafe_fetch(index : Int)
@src.unsafe_fetch(index)
end
def ==(other : self)
self.equals?(other) { |e1, e2| e1 == e2 }
end
def ==(other)
false
end
end
endThe difference here is that the source type is hardcoded to Array, and thus it's possible to only have one type parameter. Obviously, you lose the flexibility of being able to use any container type. Additionally, I did some benchmarking - there's almost no difference (maybe 5%?) in performance between the current implementation and this new version. As a result, I think I'm going to stick with ReadonlyWrapper for now, just because there's little gain from switching at the moment.
If I ever find a way to cut down on type params, though, I'd be eager to!