-
Notifications
You must be signed in to change notification settings - Fork 321
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
Align Vector API with design, add some extra functions from AoC #4026
Conversation
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Index_Sub_Range.enso
Outdated
Show resolved
Hide resolved
db516ca
to
330859f
Compare
844760d
to
017d937
Compare
d4ca398
to
81bf027
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice unification. Not very functional, but "if it quacks like a duck, it is a duck!" and Truffle should have no problem optimizing code even if different implementations of such vector-like objects pass thru.
I've added few suggestions about the API. They are just opinions.
I requested improvements in the test suites. Having a single test suite that is then executed with all suitable data types will ensure that expected behavior is the same for implementations. I'd like to see this done.
Remaining question is: how to prevent these vector-like types diverging in the future? I was thinking of using James tricks like:
var module = Context.getCurrent().getBindings("enso").invokeMember("get_module", moduleName);
var type = module.invokeMember("get_type", typeName);
var methods = type.getMemberKeys();
to verify there are no unexpected methods, parameters, etc. in all the vector-like types. This would prevent against future modifications that are done only in some of the vector-like types. This is just a suggestion, but it'd be nice to have such a signature testing infrastructure - it is going to be handy in the future (for example different implementations of SPI interfaces).
Print each element in the vector to standard output. | ||
|
||
Pair.new 1 2 . each IO.println | ||
each : (Any -> Any) -> Nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is each
really needed? I could just use map
instead (and I think I did in the past):
Pair.new 1 2 . map IO.println
works as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map
keeps a Vector of results for each execution, whereas each
discards the result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only difference is performance then, right?
In case of Pair
using or discarding the result doesn't make a difference, I think. The compiler will see that the result isn't used and will avoid allocating the new Pair
completely. Whether that is true for Vector
and List
as well would have to be checked. Chances are the Enso engine could provide the Graal compiler enough information to make map
as effective as each
.
Would then the need to have two APIs doing almost the same thing be eliminated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have a way where the compiler would know to discard the result, then yes could have a single API.
Currently, the map function will preallocate a Vector of the same size as the input for Vector.map
. Likewise, for Range
it does too. List and Pair have more specific implementations. We will have to tune memory allocations in various places in the coming months though.
@@ -70,6 +72,50 @@ spec = Test.group "Range" <| | |||
0.up_to 1 . not_empty . should_be_true | |||
0.up_to 5 . not_empty . should_be_true | |||
5.down_to 0 . not_empty . should_be_true | |||
Test.specify "should allow getting by index using at" <| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a generic test suite working on Vector
, List
, Array
(as soon as @hubertp aligns its methods) making sure the behavior is the same on all implementations of these "vector-like" objects.
Here is an example where I was comparing methods on two types. |
0997c13
to
6a3588f
Compare
29a41e0
to
93eb407
Compare
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Extensions.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Extensions.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Extensions.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Extensions.enso
Outdated
Show resolved
Hide resolved
test/Tests/src/Data/List_Spec.enso
Outdated
l.find (==2) start=2 . should_fail_with Not_Found | ||
l.find (==2) start=-1 . should_fail_with Not_Found | ||
l.find (==2) start=-2 . should_equal 2 | ||
empty.find (==1) . should_fail_with Not_Found | ||
empty.find (==1) if_missing=Nothing . should_equal Nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if I do start=100
? We should have a test for find
that checks out of bounds offsets, much like we have for index_of
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the new APIs and the direction where all of this is going, it's definitely a great step forward.
I like the changes, just in some cases I'd appreciate if we can simplify the code a bit.
Other than that - I think a few docs need some clarification (as noted) and there are a few tests that I think should be added - especially for the List
methods - since the methods are not as easy to comprehend and see if there may be any bugs lurking, it's more crucial to get them very well tested.
Fix Pair test.
Text_Spec tests for index_of.
7a40c3e
to
dea9055
Compare
Pull Request Description
Vector
Vector.sort
to beVector.sort order on by
.order
for direction argument.insert
,remove
,index_of
andlast_index_of
toVector
.start
andif_missing
arguments tofind
onVector
, and adjusted default isNot_Found
error.+
onVector
.first
,second
andlast
to error withIndex_Out_Of_Bounds
onVector
.sum
,exists
,head
,init
,tail
,rest
,append
,prepend
fromVector
.Pair
last
,any
,all
,contains
,find
,index_of
,last_index_of
,reverse
,each
,fold
andreduce
toPair
.get
toPair
.Range
first
,second
,index_of
,last_index_of
,reverse
andreduce
toRange
.at
andget
toRange
.start
andif_missing
arguments tofind
onRange
.last
andlength
ofRange
.exists
fromRange
.List
second
,find
,index_of
,last_index_of
,reverse
andreduce
toRange
.at
andget
toList
.exists
fromList
.all
short-circuit if any fail onList
.is_empty
to not compute the length ofList
.first
,tail
,head
,init
andlast
to error withIndex_Out_Of_Bounds
onList
.Others
first
,second
,last
,get
toText
.Aggregate_Column
to operate on the first column by default.contains_key
toMap
.row_count
andorder_by
.Checklist
Please include the following checklist in your PR:
Scala,
Java,
and
Rust
style guides.
./run ide build
and./run ide watch
.