Switch to Scroll-owned Goldilocks repository

[matthiasgoergens]

Also remove manual Ord implementation for Expression and use a derived implementation that does the same as the manual one used to do.

[yczhangsjtu]

For the Ord, I don't know why is there a custom implementation in the first place? Was there any special considerations?

[matthiasgoergens]

For the Ord, I don't know why is there a custom implementation in the first place? Was there any special considerations?

I only added 'derive Ord' to the upstream repo (which is now scroll owned) recently.

In general, we should do more upstream fixes, instead of just working around them.

[matthiasgoergens]

@yczhangsjtu Please tell me, if there's anything left you want me to do before you can approve? (Or let me know, if you just feel uncomfortable with the responsibility.)

[yczhangsjtu]

@yczhangsjtu Please tell me, if there's anything left you want me to do before you can approve? (Or let me know, if you just feel uncomfortable with the responsibility.)

I do feel uneasy since the default implementation may introduce a different order on the types from here, and I don't know if that might break something. But I figure it should be okay if the tests pass.

[yczhangsjtu]

For the Ord, I don't know why is there a custom implementation in the first place? Was there any special considerations?

I only added 'derive Ord' to the upstream repo (which is now scroll owned) recently.

In general, we should do more upstream fixes, instead of just working around them.

So the reason for this custom implementation was that Goldilocks didn't implement Ord, right?

[matthiasgoergens]

So the reason for this custom implementation was that Goldilocks didn't implement Ord, right?

Well, that's my best guess from reading the code and digging through the git history. There was lots of other weirdness that I can't explain, though. For example, why all the tedious cases analysis, instead of then or then_with.

[matthiasgoergens]

I do feel uneasy since the default implementation may introduce a different order on the types from here, and I don't know if that might break something. But I figure it should be okay if the tests pass.

That's a fair point to make. Do you want me to investigate, or are you happy with the existing tests?

[naure]

Are there any other differences besides Ord?

[naure]

Nice!

[matthiasgoergens]

Are there any other differences besides Ord?

Not really.

[naure]

That weird custom Ord is used to deduplicate commutative operations (b*a == a*b).

It can be replaced with the default order now.

[matthiasgoergens]

That weird custom Ord is used to deduplicate commutative operations (b*a == a*b).

It can be replaced with the default order now.

I understand that one. I was more confused by the custom reimplementation of 'then'.