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

#(1 2 3) , #(4 5 6) asSet raises an error #4807

Open
juliendelplanque opened this issue Oct 3, 2019 · 9 comments

Comments

@juliendelplanque
Copy link
Member

commented Oct 3, 2019

#(1 2 3) asSet , #(4 5 6)

does not.

@svenvc

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

What is the contract for #, though ? Should it assume similar types or not ...

'foo' , #(1 2 3).

#(1 2 43) , 'foo'.

@juliendelplanque

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2019

Well, it expects the receiver to understand #addAll: so this is why it does not work with Array.

I think it just requires that the receiver is growable and the argument is a collection for now.

I am not sure it is the right contract.

@kasperosterbye

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

The error is because #, is overwritten in SequenceableCollection from which Array inherrits.
The implementation in SequenceableCollection is wrong, as it assumes the argument to be indexable (in that is uses copyReplaceFrom:to:with:, who again use replaceFrom:to:with:startingAt: which assumes the argument collection to be a SequenceableCollection.

@kasperosterbye

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

In essence it boils down to the fact that #, is an operator which is super well defined for iterators, but is messed up in the collection library. a , b should merely be a shorthand for (a iterator , b iterator) as: a species.

@kasperosterbye

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

But to "solve" the issue, I propose to change the implementation of #, in SequencableCollection from:

, otherCollection
	"Concatenate two Strings or Collections."
	"#(2 4 6 8) , #(who do we appreciate) >>> #(2 4 6 8 who do we appreciate)"
	"((2989 storeStringBase: 16) copyFrom: 4 to: 6) , ' boy!' >>> 'BAD boys!'"

	^ self
		copyReplaceFrom: self size + 1
		to: self size
		with: otherCollection

to:

, otherCollection
	"Concatenate two Strings or Collections."
	"#(2 4 6 8) , #(who do we appreciate) >>> #(2 4 6 8 who do we appreciate)"
	"((2989 storeStringBase: 16) copyFrom: 4 to: 6) , ' boy!' >>> 'BAD boys!'"

	^ self
		copyReplaceFrom: self size + 1
		to: self size
		with:
			(otherCollection isSequenceable
				ifTrue: [ otherCollection ]
				ifFalse: [ otherCollection asArray ])

Or implement a asSequencable to avoid the type test.

@juliendelplanque

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

Does it even makes sense to have concatenation operator on other collections than sequenceable ones?

@svenvc

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

Indeed, there is #union:

@juliendelplanque

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

I mean, concatenation induces that the collections are sequenceable.

In general, to concatenate two collections is different from making the union.
The union comes from set theory and the result does not contain duplicated items.
Concatenation do not care about items duplication.

@kasperosterbye

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

It is not clear to me what aSet , aCol should mean. I can see aSeq , aCol to be a sequence with aSeq followed by the elements of aCol - in their right order if aCol is ordered, or any order if aCol is not orderd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.