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

Remove -release option from the Scala 3 PC #3812

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Apr 10, 2022

Previously, in a number of scenarios this option would break the presentation compiler for Scala 3. Now we filter it out since it's not important for the frontend phases.

PReviously, in a number of scenarios this option would break the presentation compiler for Scala 3. Now we filter it out since it's not important for the frontend phases.
Copy link
Member

@tanishiking tanishiking left a comment

Choose a reason for hiding this comment

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

LGTM

-release              Compile code with classes specific to the given version
                      of the Java platform available on the classpath and emit
                      bytecode for this version.

Indeed it's not important for frontend phases 👍

Comment on lines +63 to +68
private def removeDoubleOptions(options: List[String]): List[String] =
options match
case head :: _ :: tail if forbiddenDoubleOptions(head) =>
removeDoubleOptions(tail)
case head :: tail => head :: removeDoubleOptions(tail)
case Nil => options
Copy link
Member

Choose a reason for hiding this comment

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

Why not

Suggested change
private def removeDoubleOptions(options: List[String]): List[String] =
options match
case head :: _ :: tail if forbiddenDoubleOptions(head) =>
removeDoubleOptions(tail)
case head :: tail => head :: removeDoubleOptions(tail)
case Nil => options
options.filterNot(forbiddenDoubleOptions)

?

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, nevermind, removeDoubleOptions transforms List("-release", "8", ...) to List(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to get a bit more creative here 😅

Copy link
Member

@kpodsiad kpodsiad left a comment

Choose a reason for hiding this comment

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

Great!

@tgodzik tgodzik merged commit 66fc1ce into scalameta:main Apr 12, 2022
@tgodzik tgodzik deleted the remove-release-flag branch April 12, 2022 10:46
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