Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace functor IntervalFunctor with a module #275

Closed
jerhard opened this issue Jul 8, 2021 · 5 comments
Closed

Replace functor IntervalFunctor with a module #275

jerhard opened this issue Jul 8, 2021 · 5 comments
Assignees
Labels
cleanup Refactoring, clean-up

Comments

@jerhard
Copy link
Member

jerhard commented Jul 8, 2021

Currently, the IntervalFunctor takes an IntOps module to allow instantiations of the interval domain to work with either int64 or bigints. The only places that still rely on the intervals using int64 internally are the ranges for DefExc and Enums, but these can be patched to work with bigints.

A benefit would be that the code structure gets simpler by getting rid of the functor, and additionally, the functions in IntDomain.Size would work on bigints more uniformly.

@jerhard jerhard self-assigned this Jul 8, 2021
@jerhard jerhard added the cleanup Refactoring, clean-up label Jul 8, 2021
@sim642
Copy link
Member

sim642 commented Jul 9, 2021

The only places that still rely on the intervals using int64 internally are the ranges for DefExc and Enums, but these can be patched to work with bigints.

But these ranges represent the number of bits, not the values themselves, right? Seems way overkill to use bigints for numbers like 64 or -63.
Tbh, it'd make even more sense to switch them from int64 to int (which Size.bits returns anyway without problem), which has even less memory overhead in the OCaml runtime.

A benefit would be that the code structure gets simpler by getting rid of the functor

Does it actually get simpler by any extent? First, you just get rid of the functor's argument and hardcode module Ints_t = IntOps.BigIntOps. Second, you could inline all the Ints_t calls, but I'd argue it's not any easier to read Big_int_Z.add_big_int instead of Ints_t.add, etc.

the functions in IntDomain.Size would work on bigints more uniformly.

I suppose you're referring to the fact that many of them still return int64s, but also have bigint returning versions. Surely the int64 versions could be removed and the places that still depend on that can simply use IntOps.of_bigint at their own risk of erroring (which happens currently anyway as Size raises Not_in_int64 in some cases).

So given the generality of the already implemented functor, I don't really see the reason to throw it away. Especially now that in #251 the flattened int domain has also been generalized to any IntOps.

I think there are other parts of IntDomain that are a much bigger source of confusion

@vogler
Copy link
Collaborator

vogler commented Jul 9, 2021

But these ranges represent the number of bits, not the values themselves, right? Seems way overkill to use bigints for numbers like 64 or -63.
Tbh, it'd make even more sense to switch them from int64 to int (which Size.bits returns anyway without problem), which has even less memory overhead in the OCaml runtime.

Agree

the functions in IntDomain.Size would work on bigints more uniformly.

I suppose you're referring to the fact that many of them still return int64s, but also have bigint returning versions. Surely the int64 versions could be removed and the places that still depend on that can simply use IntOps.of_bigint at their own risk of erroring (which happens currently anyway as Size raises Not_in_int64 in some cases).

I thought they only return big_int where needed, and use int or int64 for number of bits etc.
The code was written at a time when all int domains used int64 instead of big_int.
So, yes, some cleanup there would prob. be better.

@michael-schwarz
Copy link
Member

What is the state here? Do we still want to do this, or should we close the issue?

@sim642
Copy link
Member

sim642 commented May 13, 2022

My opinion is still that it should remain as a functor. In the light of #734 it might even be beneficial to further extract and generalize the interval domain, such that certain parts could be reused for floats as well.

@jerhard jerhard closed this as completed May 13, 2022
@jerhard
Copy link
Member Author

jerhard commented May 13, 2022

I closed the issue as there are still two instances of the functor (with bigints and with 64 bit-ints).

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cleanup Refactoring, clean-up
Projects
None yet
Development

No branches or pull requests

4 participants