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

Range #551

Merged
merged 11 commits into from
Oct 19, 2022
Merged

Range #551

merged 11 commits into from
Oct 19, 2022

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 jcp19 and Felalolf 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?

src/test/scala/viper/gobra/parsing/GoParserUnitTests.scala Outdated Show resolved Hide resolved
src/main/java/viper/gobra/frontend/GobraLexer.java Outdated Show resolved Hide resolved
@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?

src/main/java/viper/gobra/frontend/GobraParser.java Outdated Show resolved Hide resolved
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

src/main/scala/viper/gobra/frontend/Desugar.scala Outdated Show resolved Hide resolved
src/main/scala/viper/gobra/frontend/Desugar.scala Outdated Show resolved Hide resolved
src/main/scala/viper/gobra/frontend/Desugar.scala Outdated Show resolved Hide resolved
src/main/scala/viper/gobra/frontend/Desugar.scala Outdated Show resolved Hide resolved
src/main/scala/viper/gobra/frontend/Desugar.scala Outdated Show resolved Hide resolved
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