`BigInt` support and refactors #5

alexmozaidze · 2021-11-17T22:56:23Z

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 · 2021-11-19T08:46:08Z

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 · 2021-11-20T10:43:41Z

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 · 2021-11-20T16:39:27Z

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

brundonsmith · 2021-11-23T04:16:43Z

src/model.rs

-      Value::False => false,
-      _ => true,
+    pub fn is_truthy(&self) -> bool {
+        !matches!(self, Value::List(List::NIL) | Value::False) // Matches not those values


Better yet would be:

self == &Value::False || self == &Value::NIL

More straightforward, and I imagine it might also be more performant

brundonsmith · 2021-11-23T04:30:29Z

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 · 2021-11-24T02:45:30Z

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 · 2021-11-25T19:10:15Z

src/parser.rs

+                if !stack.is_empty() {
                    if let ParseTree::List { vec, quoted: _ } = stack.last_mut().unwrap() {


Even better might be:

if let Some(last) = stack.last_mut() { if let ParseTree::List { vec, quoted: _ } = last {

brundonsmith · 2021-11-25T19:33:32Z

src/default_environment.rs

+
+                    Ok(Value::List(res.into_iter().map(Value::Int).collect::<List>()))
+                } else {
+                    Ok(Value::List((start..=end).map(Value::Int).collect::<List>()))


Oh wow, was this logic broken before? (start..end vs start..=end)

Actually- I think the exclusive behavior was intended, since this is how ranges tend to work in other languages:

(range 0 10) '(0, 1, 2, 3, 4, 5, 6, 7, 8, 9)

So let's remove the = here, and also change the <= to < in the BigInt version

brundonsmith · 2021-11-25T19:43:01Z

src/default_environment.rs

+                    let mut i = start.clone();
+                    let mut res = Vec::with_capacity((end.clone() - start)
+                        .to_usize()
+                        .ok_or(RuntimeError::new("Failed converting `BigInt` to `usize`"))?
+                    );
+
+                    while i <= end {
+                        res.push(i.clone());
+                        i += 1;
+                    }
+
+                    Ok(Value::List(res.into_iter().map(Value::Int).collect::<List>()))


It's weird that BigInt doesn't seem to implement Step. Still, I can't help but feel there might be a simpler way to do this? If not, this is probably fine

Ah hmm, that's disappointing
rust-num/num-bigint#68

Yeah, I asked about it in Rust discord server, they gave me the same answer

brundonsmith · 2021-11-25T19:48:37Z

src/interpreter.rs

                        argnames,
                        body,
                    }))
                }

                Value::Symbol(symbol) if symbol == "quote" => {
-                    let exp = list.cdr().car()?.clone();
+                    let exp = list.cdr().car()?;


Just curious, did you have some sort of tool that helped you find all these unneeded Rc .clone()s? I suspected there were some but had no way to identify them all en masse

cargo clippy, I use NeoVim with Ale to run this command every time I save a file

Huh. My editor shows clippy hints inline, but I've never seen that particular rule

brundonsmith · 2021-11-25T19:51:02Z

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 · 2021-11-26T07:30:09Z

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 · 2021-11-26T22:03:04Z

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 · 2021-11-26T22:05:07Z

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

…tching

alexmozaidze · 2021-11-27T14:33:44Z

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.

alexmozaidze changed the title ~~Bigint and refactors~~ BigInt support and refactors Nov 17, 2021

brundonsmith reviewed Nov 23, 2021

View reviewed changes

BigInt support, Configurable Int and Float, and refactors

3349072

🦀

0c55044

brundonsmith reviewed Nov 25, 2021

View reviewed changes

Made range exclusive, and refactored if statement to use pattern ma…

d9cc3c6

…tching

brundonsmith merged commit 3e1e6cc into brundonsmith:master Nov 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`BigInt` support and refactors #5

`BigInt` support and refactors #5

alexmozaidze commented Nov 17, 2021 •

edited

Loading

alexmozaidze commented Nov 19, 2021 •

edited

Loading

alexmozaidze commented Nov 20, 2021 •

edited

Loading

alexmozaidze commented Nov 20, 2021

brundonsmith Nov 23, 2021 •

edited

Loading

brundonsmith commented Nov 23, 2021

alexmozaidze commented Nov 24, 2021 •

edited

Loading

brundonsmith Nov 25, 2021

brundonsmith Nov 25, 2021

brundonsmith Nov 25, 2021

brundonsmith Nov 25, 2021

brundonsmith Nov 25, 2021

alexmozaidze Nov 26, 2021

brundonsmith Nov 25, 2021

alexmozaidze Nov 26, 2021

brundonsmith Nov 26, 2021

brundonsmith commented Nov 25, 2021 •

edited

Loading

alexmozaidze commented Nov 26, 2021 •

edited

Loading

brundonsmith commented Nov 26, 2021

brundonsmith commented Nov 26, 2021

alexmozaidze commented Nov 27, 2021

		if !stack.is_empty() {
		if let ParseTree::List { vec, quoted: _ } = stack.last_mut().unwrap() {

BigInt support and refactors #5

BigInt support and refactors #5

Conversation

alexmozaidze commented Nov 17, 2021 • edited Loading

alexmozaidze commented Nov 19, 2021 • edited Loading

alexmozaidze commented Nov 20, 2021 • edited Loading

alexmozaidze commented Nov 20, 2021

brundonsmith Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

brundonsmith commented Nov 23, 2021

alexmozaidze commented Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brundonsmith commented Nov 25, 2021 • edited Loading

alexmozaidze commented Nov 26, 2021 • edited Loading

brundonsmith commented Nov 26, 2021

brundonsmith commented Nov 26, 2021

alexmozaidze commented Nov 27, 2021

`BigInt` support and refactors #5

`BigInt` support and refactors #5

alexmozaidze commented Nov 17, 2021 •

edited

Loading

alexmozaidze commented Nov 19, 2021 •

edited

Loading

alexmozaidze commented Nov 20, 2021 •

edited

Loading

brundonsmith Nov 23, 2021 •

edited

Loading

alexmozaidze commented Nov 24, 2021 •

edited

Loading

brundonsmith commented Nov 25, 2021 •

edited

Loading

alexmozaidze commented Nov 26, 2021 •

edited

Loading