Execute 128bits Hasher specs that are now passing

[luislavena]

It seems i128 and u128 hash specs were left out as pending. This change runs them as latest Crystal is cable to compute equal hashes.

Ref #5276

/cc @akzhan

[luislavena]

Ah, Travis CI failed 😞

Seems associated with the version of Crystal used within Docker.

Will investigate.

[luislavena]

Ah, this will need to wait until next release.

Will rebase once 0.24.1 is out.

Sorry for the noise.
❤️ ❤️ ❤️

[straight-shoota]

@luislavena I think this can be merged now but please rebase on master to get new CI builds.

[asterite]

Oops, Int128 isn't supported everywhere, as I mentioned some times already.

Let's remove Int128 from the language, unless someone has an idea how to include all those missing symbols (like __modti3 and __umodti3) in an easy way.

[luislavena]

@asterite I was going to suggest exclude Int128 for x86, seems appears to be the only platform in the CI that is not supported.

I don't have a solution to include these missing functions, but I would rather have it on specific platforms than not having it at all.

Cheers.

[asterite]

But then someone can use Int128, but when trying to run the code in another platform it will fail compilation. I prefer code to be portable instead of adding a feature that very few will use.

[straight-shoota]

Rust implements the missing functions directly in Rust: https://github.com/rust-lang/rfcs/blob/master/text/1504-int128.md#runtime-library-support

[ysbaddaden]

Let's drop i128 support: it's shady, broken, target dependent and not portable...

[luislavena]

@asterite @ysbaddaden I have nothing against or in favor of removal, but then wonder why was added in the first place? If memory serves me well, I thought was to deal with time stuff? Is that no longer true and another solution was achieved?

[asterite]

@luislavena I was just bored and wanted to see if it was easy to implement 😊

Maybe Time was a use case, I don't know. In any case, the current implementation is working well and doesn't need an Int128 type.

[Sija]

@asterite Int128 is gonna gain more widespread adoption in the coming months/years, not less. Wouldn't be better to finish the implementation (implementing missing functions in Crystal if needed), instead of just dropping the whole feature?

[asterite]

@Sija Sure. PRs are welcome :-)

[straight-shoota]

According to the Rust RFC they needed a compiler capable of handling 128-bit integers to implement and compile the missing functions for 32-bit platforms. If we remove it now, we'd first have to re-add it incomplete again to be able to implement these functions. Since Int128 is already there anyway and at least partly working I'd suggest to avoid complications and leave it in (and this PR open).

[RX14]

Well, I really want to move Time to 128bit integers, because as far as i'm concerned the current implementation is terrible. Simple things like trying to multiply a time span now go via a floating point type and hence loose accuracy. I've seen this practically in predict.cr, I've had to back off the tolerance of my spec suite by several orders of magnitude to allow for the extra errors in the new time types. It's unacceptable.

[RX14]

Rust has moved it's reimplementation of compiler-rt outside of it's stdlib so it can be built seperately. It could be useful to consider just building rust's compiler-rt and grabbing the int128 functions we need from there: https://github.com/rust-lang-nursery/compiler-builtins

Until then i'd just gate these specs on :bits64

[RX14]

We probably want to modify the {u,}int_spec.cr to deal with 128bit arithmetic too. But this is fine to merge for now as it's an improvement.

[ysbaddaden]

I disagree: if a feature isn't available or broken on some targets, then it should be disabled for these targets, not left broken.

Furthermore the rust compiler-rt port is tailored for rust and likely incompatible in some cases. i128 support may be nice, but too far to reach to even think about having anything in stdlib rely on it anytime soon —it took years of effort to stabilize in rust.

[RX14]

the rust compiler-rt port is tailored for rust and likely incompatible in some cases

LLVM emits the code which calls compiler-rt. It won't be incompatible. It can't be. Even if it was, software 128bit arithmetic is software 128bit arithmetic, and we could copy the algorithm at least.

128bit support in rust wasn't that much code, they just spent a year testing it. We don't have the same concept of "unstable features" rust has.

[bcardiff]

This PR enables the spec that were already written. Lets enable them. But let's see how the story of 128 for other platforms goes when the times comes.

I second that in mid term the functionality and api needs to be the same in platform and os, even at the cost of loosing some functionality (at least in the main apis).