search

RustPython / Parser

MIT License
67 stars 26 forks source link

Replace num-bigint with malachite-bigint #18

Closed qingshi163 closed 1 year ago

youknowone commented 1 year ago

I wish we can see https://github.com/RustPython/RustPython/pull/4952 success before merging this PR

qingshi163 commented 1 year ago

We need to check compatibility for RustPython and Ruff, also the benchmark mentioned by @DimitrisJim

qingshi163 commented 1 year ago

I see the new merged pr #25 witch import pyo3 module with num-bigint feature. I did not work with pyo3 at all, don't know how heavy it will affect this pr.

@youknowone if possible can you help the rebase?

youknowone commented 1 year ago

sure, i will

youknowone commented 1 year ago

@qingshi163 is num-bigint to malachite-bigint conversion possible?

qingshi163 commented 1 year ago

@qingshi163 is num-bigint to malachite-bigint conversion possible?

sure it is convertable with very low cost. I will add the convertion function with feature flag in malachite-bigint

youknowone commented 1 year ago

Thank you! That seems the only missing component.

qingshi163 commented 1 year ago

I have done the conversions on the malachite-bigint side. @youknowone

youknowone commented 1 year ago

Great! Patching Ruff to use malachite_bigint was unbelievably easy.

youknowone commented 1 year ago

Everything seems fine and working great!

@qingshi163 I will finish this PR. A little bit of concern is From<num_bigint::BigInt> for malachite_bigint::BigInt is supported, but not AsRef<malachite_bigint::BigInt> for num_bigint::BigInt. When using our AST, we usually can get &BigInt but not BigInt except for Fold, which is not very popular feature. Because num-bigint is still a fundamental part of Rust environment (yet), supporting both bigint library for a while will make sense. num-bigint and malachite-bigint will be exclusive features and malachite-bigint will be the default.

I hope malachite-bigint replace more num-bigint use case.