Skip to content

Range#551

Merged
Dspil merged 11 commits intomasterfrom
slice_range
Oct 19, 2022
Merged

Range#551
Dspil merged 11 commits intomasterfrom
slice_range

Conversation

@Dspil
Copy link
Contributor

@Dspil Dspil commented Oct 15, 2022

Range for slices/arrays is reworked as discussed.
Range for maps is also implemented.
Variables declared with := in range expressions can be shared now (this is the case with Go)

@Dspil Dspil changed the title Slice range Range Oct 15, 2022
@Dspil Dspil requested review from Felalolf and jcp19 October 15, 2022 18:25
Copy link
Contributor

@jcp19 jcp19 left a comment

Choose a reason for hiding this comment

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

Just identified minor stuff rn. I will take a more thorough look tomorrow. Why is genparser.sh part of the changeset?

@Dspil
Copy link
Contributor Author

Dspil commented Oct 17, 2022

Why is genparser.sh part of the changeset?

I think it is because I added execute permissions to it to run it and git captured that too.

Copy link
Contributor

@Felalolf Felalolf left a comment

Choose a reason for hiding this comment

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

A few smaller comments. I did not check the desugaring thoroughly, but just that it does not break existing stuff.

Copy link
Contributor

@jcp19 jcp19 left a comment

Choose a reason for hiding this comment

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

It is a bit hard for me to follow the Desugarer, as it seems very long and repetitive. Is there a way we can nicely refactor this into smaller, easy to grok methods?

Comment on lines +295 to +299
underlyingType(exprType(exp)) match {
case _: SliceT | _: ArrayT => IntT(config.typeBounds.Int)
case MapT(key, _) => SetT(key)
case t => violation(s"type $t is not supported for range")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a repeated pattern, maybe we could abstract this in a method

Copy link
Contributor

@jcp19 jcp19 left a comment

Choose a reason for hiding this comment

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

We can merge it by me

@Dspil Dspil merged commit a573322 into master Oct 19, 2022
@Dspil Dspil deleted the slice_range branch October 19, 2022 17:45
@jcp19 jcp19 mentioned this pull request Jan 17, 2023
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