`BigInt` support and refactors

[alexmozaidze]

Added BigInt support, along with compile-time configurable types for Value::Int and Value::Float. I did some code refactoring in form of adding new(impl Into<String>) method to error types and changing manual error constructions with constructions with the said new method. Also, I've added factorial function in order to test BigInt, but to be honest I just like seeing big numbers draw over my screen :D

In order to do some of the crazy shit I did in order to make BigInt work nicely (and kill readability), I added cfg-if, num-traits and num-bigint as dependencies. The num-traits and num-bigint are compiled if and only if feature bigint is set. I am relatively new to Rust and if you have any suggestions and/or corrections, I'd like to hear them!

[alexmozaidze]

In the commit above I also fixed the repeat function. I forgot to test it with BigInt and compiler was complaining that it couldn't compare a primitive with BigInt, so I added an extra cfg_if! to convert primitive to BigInt. I forgot to mention that little fix in the commit message :/

[alexmozaidze]

I think it may be possible to decrease amount of cfg_if! blocks if I make num-traits a required dependency. Should I do that or is it better to leave things as they are?

[alexmozaidze]

I guess I'll just do it to improve readability. An extra dependency won't do much harm.

[brundonsmith]

Hi, so I was wondering what was up with all the reformatting at first, and then I realized you ran it through cargo fmt. That seems like a reasonable thing to do, so I went ahead and did that in trunk, however it makes the diffs hard to read in this PR.

Several things that I noticed in here seem reasonable, but it's tough to review in this state. Now that trunk has been reformatted, I was wondering if you could rebase so the diff will be cleaner?

I will also say I'm concerned about adding dependencies; I really like the fact that this crate currently has zero dependencies. I don't know how important that actually is, per se, but it feels nice. With that said I appreciate the build-time flag for the main dependency, and the other two seem like they would have little to no runtime footprint, so I might be willing to merge the BigInt stuff. I'd definitely be willing to merge RuntimeError::new and some of the other minor improvements, once I can properly read through the diffs.

[alexmozaidze]

I rebased the thing, I also excluded functions I added to default_environment.rs as they were kind of not needed (factorial and feature). I brought back the cfg_if salad and now it compiles BigInt runtime dependencies only if user decides to enable bigint feature.

[brundonsmith]

One final general comment: I don't love all the special-casing for BigInt (and I see that you don't either), but since it happens at compile-time I think I can live with it, and I also suspect there might be some opportunities to eliminate it via traits; not sure. I'll take a whack at that after this is merged.

Assuming there isn't a way to eliminate it, though, I'm concerned about making sure it gets tested. Do you know if it's possible to test code under different cfg configurations?

[alexmozaidze]

Yeah, it's possible to run feature-dependant tests. With cfg_if! it goes something like this

cfg_if! {
    if #[cfg(feature = "bigint")] {
        #[test]
        fn bigint_specific_test() {
            // Do some tests with BigInt
        }
    } else {
        #[test]
        fn general_test() {
            // Do some tests in general
        }
    }
}

or with vanilla cfg

#[cfg(feature = "bigint")]
#[test]
fn bigint_specific_test() {
    // Some tests here
}

#[cfg(not(feature = "bigint"))]
#[test]
fn general_test() {
    // Some tests there
}

[brundonsmith]

Yeah- so that runs the test if the flag is enabled, but what I'd like is to run existing tests as if those feature flags were set, as part of the normal cargo test. Not sure if that's possible.

I won't let it hold up the merge, but if you know a way to do that I'd love to hear about it

[brundonsmith]

Once those first two changes I suggested have been made, this should be good to go!

[alexmozaidze]

I don't think that's possible because then compiler would have to test 2 separate versions of the program (one with and one without bigint), if we run cargo --all-features then IntType would be BigInt because it's the first check in cfg_if salad (same with FloatType - it would be f64), but I think it should be possible to chain 2 commands in shell like this cargo test && cargo test --features=bigint.