Efficient implementation of coalesce() and entries()

[gavinking]

These functions should simply wrap their argument sequence. There's no need to allocate a whole new array.

[chochos]

I'm having second thoughts about this optimization. If an ArraySequence is passed that is being used elsewhere, it gets modified. The optimization should only work for an ArraySequence that is created by the compiler to wrap the sequenced args. Which brings the question, why aren't Ceylon sequenced args compiled as Java sequenced args?

Sent via Hubroid

[gavinking]

Perhaps we should change the return types of these functions to plain Iterable to be able to get a really decent-performance implementation.

[quintesse]

Where is it possible to change ArraySequence? I thought it was always immutable?

[chochos]

it's supposed to be immutable, but the Java impl has a toArray() method that returns its internal array, so you can mess around with it directly.

[chochos]

If we change its return type to Iterable we could just leave the second option I had that returns an AbstractIterable that skips over nulls - although that could be a problem if the argument is a mutable collection...

[gavinking]

Where is it possible to change ArraySequence? I thought it was always immutable?

Right.

it's supposed to be immutable, but the Java impl has a toArray() method that returns its internal array, so you can mess around with it directly.

That's a bug I suppose. At best it should be @Ignore, at worst it's dangerous and should simply be removed.

[quintesse]

I don't think it's visible to Ceylon, but it would be to Java. So we could override it with a version that makes a copy. But I don't think we should take into account every trick people could use to make things fail. Once you pass things to Java all bets are off.

Sent via Hubroid

[gavinking]

So, after some reflection on this issue, I'm now inclined to think that we should simply remove these functions and replace them by members of Iterable, as partially proposed by #116. Since "entries" sounds a bit ambiguous for a Map, it might be better to call it "indexed". So we would get:

Iterable<Element&Object> coalesced;
Iterable<Integer->Element&Object> indexed;

The advantages of this approach:

• they're more convenient to use in an IDE, and easier to find for new users,
• they can be efficiently implemented on Iterable, and
• they can be covariantly refined on subtypes.

So I think this is the right approach.

[gavinking]

I don't think it's visible to Ceylon, but it would be to Java.

If it's not marked @Ignore, it's visible in Ceylon, no?

[gavinking]

If it's not marked @Ignore, it's visible in Ceylon, no?

Oh, it's not because ArrayList itself is not shared right now. Note that I've suggested changing that.

But I don't think we should take into account every trick people could use to make things fail. Once you pass things to Java all bets are off.

I don't agree with this. Our immutable classes should protect their internal state from mutation.

[chochos]

FTR the class used to wrap sequenced args is ArraySequence, not ArrayList. Again, why aren't Ceylon sequenced args compiled into Java sequenced args? Sorry to ask this so late in the game, but I'm new to ceylonc, so surely there must be very valid reasons (maybe default args, I don't know), I'd just like to know.

[gavinking]

A Java sequenced arg is a Java array. Ours is a Ceylon Iterable. If we compiled to a java varag method, we would lose the laziness we have today.

Sent from my iPhone

On Jul 3, 2012, at 5:35 PM, Enrique Zamudioreply@reply.github.com wrote:

FTR the class used to wrap sequenced args is ArraySequence, not ArrayList. Again, why aren't Ceylon sequenced args compiled into Java sequenced args? Sorry to ask this so late in the game, but I'm new to ceylonc, so surely there must be very valid reasons (maybe default args, I don't know), I'd just like to know.

---

Reply to this email directly or view it on GitHub:
#14 (comment)

[quintesse]

Oh, it's not because ArrayList itself is not shared right now. Note that I've suggested changing that.

What would that give us? The idea was that the actual way it's implemented was irrelevant, it's "a Sequence".

I don't agree with this. Our immutable classes should protect their internal state from mutation.

I agree, but there are cases where I hardly think that's doable. We should prevent it by means of publicly available (= non-private) methods but the moment you start using tricks you just can't do much, at least not without great cost. How would we ever detect somebody changing the internal array of that class if it was accessed using introspection?

Like I said, the simplest thing would be just to just override the toArray() to return a copy (as long as we don't use the toArray() ourselves at least, I'm not sure about that), that way we prevent obvious way to access the internal array.

[quintesse]

PS: ArraySequence.toArray() is not used anywhere in the code an can be safely removed. (Maybe we should just do so to prevent any problems)

[gavinking]

Yes, I think we should remove it.

Sent from my iPhone

On Jul 3, 2012, at 6:47 PM, Tako Schotanusreply@reply.github.com wrote:

PS: ArraySequence.toArray() is not used anywhere in the code an can be safely removed. (Maybe we should just do so to prevent any problems)

---

Reply to this email directly or view it on GitHub:
#14 (comment)

[chochos]

And what do we do about coalesce()? Really remove it and add Iterable.coalesced() as per #116?

[gavinking]

On Jul 3, 2012, at 6:27 PM, Tako Schotanusreply@reply.github.com wrote:

Oh, it's not because ArrayList itself is not shared right now. Note that I've suggested changing that.

What would that give us? The idea was that the actual way it's implemented was irrelevant, it's "a Sequence".

Well for a start it means you can pass around a ref to ArraySequence to use it to create sequences.

And ArrySequence already does (or should) hide its internal implementation.

ArraySequence is no different to Singleton or Range in these respects. We should not encourage people to think {} is a somehow magical thing defined outside the type system. It's just a cute abbreviation.

[gavinking]

Right, that's what I just said, I think...

Unless you think it's independently useful as a convenience thing to write stuff like:

value name=" ".join(coalesce(first, middle, initial));

I can see that being very occasionally useful, I suppose...

Sent from my iPhone

On Jul 3, 2012, at 7:01 PM, Enrique Zamudioreply@reply.github.com wrote:

And what do we do about coalesce()? Really remove it and add Iterable.coalesced() as per #116?

---

Reply to this email directly or view it on GitHub:
#14 (comment)

[gavinking]

Actually I suppose it does make sense in general to have a toplevel fn with a sequenced param that has the same semantics as a method if Iterable. It saves a noisy call to elements().

Sent from my iPhone

On Jul 3, 2012, at 7:07 PM, Gavin King gavin.king@gmail.com wrote:

Right, that's what I just said, I think...

Unless you think it's independently useful as a convenience thing to write stuff like:

value name=" ".join(coalesce(first, middle, initial));

I can see that being very occasionally useful, I suppose...

Sent from my iPhone

On Jul 3, 2012, at 7:01 PM, Enrique Zamudioreply@reply.github.com wrote:

And what do we do about coalesce()? Really remove it and add Iterable.coalesced() as per #116?

---

Reply to this email directly or view it on GitHub:
#14 (comment)

[chochos]

ok so we'll leave coalesce(), just have to decide what to do about its implementation (revert it to the way it was, or leave it as just the Iterable). I'll implement Iterable.coalesced.

[gavinking]

ok so we'll leave coalesce(), just have to decide what to do about its implementation

I would redefine coalesce() and entries() like so:

Iterable<Element&Object> coalesce<Element>(Element... elements) {
    return elements.coalesced;
}

Iterable<Integer->Element&Object> entries<Element>(Element... elements) {
    return elements.indexed;
}

I suppose that's the most generally-useful definition.

[chochos]

OK if we change the signature to that then we can leave the Iterable implementation I already did for coalesce and do something similar for entries.

[gavinking]

@chochos Thanks man, looks great.