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

Adds a Num.restrictToInterval function #6286

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

KilianVounckx
Copy link
Contributor

This PR is the result from this issue and this zulip thread.

crates/cli_testing_examples/benchmarks/CFold Outdated Show resolved Hide resolved
crates/compiler/builtins/roc/Num.roc Outdated Show resolved Hide resolved
@Anton-4
Copy link
Collaborator

Anton-4 commented Dec 16, 2023

We don't run expects tests for Num.roc yet. I need to do #6289 before we merge this.

Comment on lines +876 to +878
## restrictToInterval 10 { startAt: 3, endAt: 8 } == 8

## restrictToInterval 5 { startAt: 8, endAt: 3 } == 5
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## restrictToInterval 10 { startAt: 3, endAt: 8 } == 8
## restrictToInterval 5 { startAt: 8, endAt: 3 } == 5
## restrictToInterval 10 { startAt: 3, endAt: 8 } == 8
##
## restrictToInterval 5 { startAt: 8, endAt: 3 } == 5

## restrictToInterval 1 { startAt: 8, endAt: 3 } == 3
## restrictToInterval 10 { startAt: 8, endAt: 3 } == 8
## ```
## You may know this function as `clamp` in other languages.
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove this from the Roc documentation. On the docs website, there is a way to list functions with other names in other languages:

<h1>Different Names</h1>

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see you've already done the later.

Comment on lines +885 to +886
if endAt > startAt then
restrictToInterval x { startAt: endAt, endAt: startAt }
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right to me. Did you mean endAt < startAt?

Separately, I wonder if that's the desired behavior. Maybe we should just clamp to the startAt value in this case?

Comment on lines +2 to +3
let Num.306 : I64 = lowlevel NumAdd #Attr.2 #Attr.3;
ret Num.306;
Copy link
Member

Choose a reason for hiding this comment

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

@folkertdev I wonder if there's a way we can figure out to avoid these diffs everytime a builtin changes. I don't have any good ideas, but maybe something to keep in the back of our minds.

Copy link
Contributor

Choose a reason for hiding this comment

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

richard's ideas for making module depend solely on their contents for caching reasons should do that I think? or at least we can try to make that work in that context.

Copy link

Thank you for your contribution! Sometimes PRs end up staying open for a long time without activity, which can make the list of open PRs get long and time-consuming to review. To keep things manageable for reviewers, this bot automatically closes PRs that haven’t had activity in 60 days. This PR hasn’t had activity in 30 days, so it will be automatically closed if there is no more activity in the next 30 days. Keep in mind that PRs marked Closed are not deleted, so no matter what, the PR will still be right here in the repo. You can always access it and reopen it anytime you like!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants