Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat(corelib): add RangeBounds and Bound #7006

Closed
wants to merge 1 commit into from

Conversation

cairoIover
Copy link
Contributor

@cairoIover cairoIover commented Jan 5, 2025

With the addition of new range operators and range types (WIP in #6972) , we are going to have different kinds of bounds for ranges.

Implements the Bound enum (representing all 3 kinds of possible bounds) and the RangeBounds associated trait to get the bounds of a range.

Later this will be useful to abstract some inner impls (like whether a range contains a number or not) an use a single common implementation.

I left out some comments regarding internal bugs to discuss.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it is useful only for contains i rather just have an implementation for contains.

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @cairoIover)


corelib/src/ops/range.cairo line 263 at r1 (raw file):

                Bound::Excluded(end) => item < end,
                Bound::Unbounded => true,
            })

implementing the basic implementation is more efficient.

Suggestion:

        start <= item && item < end

Copy link
Contributor Author

@cairoIover cairoIover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

understandable. I was thinking it would be interesting to make reasoning for contains based on bounds types, but we can also go for simplicity and avoid abstractions. I can close

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @orizi)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

until we find further use cases - i think this is preferable.

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @cairoIover)

@cairoIover cairoIover closed this Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants