Skip to content

Rename scala.quoted.staging.{Toolbox => Compiler} #11129

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

Merged
merged 2 commits into from
Jan 18, 2021

Conversation

nicolasstucki
Copy link
Contributor

The purpose is to make it clearer what this abstraction contains.
This would also help users understand that if they want to use staging within a macro they need to create a second compiler (usually not a good idea).

@nicolasstucki nicolasstucki self-assigned this Jan 15, 2021
@nicolasstucki nicolasstucki force-pushed the rename-staging-toolbox branch from 1291e09 to cbc497d Compare January 15, 2021 11:42
@nicolasstucki nicolasstucki marked this pull request as ready for review January 15, 2021 13:30
@nicolasstucki nicolasstucki added the release-notes Should be mentioned in the release notes label Jan 18, 2021
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM

@nicolasstucki nicolasstucki changed the title Rename scala.quoted.staging.{Toolbox => JITCompiler} Rename scala.quoted.staging.{Toolbox => Compiler} Jan 18, 2021
@nicolasstucki nicolasstucki force-pushed the rename-staging-toolbox branch 2 times, most recently from 44d083d to 3c215db Compare January 18, 2021 10:58
@nicolasstucki
Copy link
Contributor Author

@liufengyun could you double-check that I did not miss something while renaming from JITCompiler to Compiler

The purpose is to make it clearer what this abstraction contains.
This would also help users understand that if they want to use staging within a macro they need to create a second compiler (usually not a good idea).
@nicolasstucki nicolasstucki force-pushed the rename-staging-toolbox branch from 3c215db to 1e0475c Compare January 18, 2021 12:15
scala> implicit def toolbox: Toolbox = Toolbox.make(getClass.getClassLoader)
def toolbox: quoted.staging.Toolbox
scala> import quoted.staging.{Compiler => StagingCompiler, _}
scala> implicit def compiler: StagingCompiler = StagingCompiler.make(getClass.getClassLoader)
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 rename it here due to some strange interaction with java.lang.Compiler import

scala> import quoted._
scala> import quoted.staging._
scala> implicit def compiler: Compiler = Compiler.make(getClass.getClassLoader)
1 | implicit def compiler: Compiler = Compiler.make(getClass.getClassLoader)
  |                        ^^^^^^^^
  |                       Reference to Compiler is ambiguous,
  |                       it is both imported by import quoted.staging._
  |                       and imported subsequently by import java.lang.{...}

Copy link
Contributor

Choose a reason for hiding this comment

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

StagingCompiler sounds like an even better name than Compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a more general issue. I opened #11146.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If staging compiler sound better, maybe we could rewrite the docs to use

import scala.quoted._

given staging.Compiler = staging.Compiler.make(...)

staging.run {
  ...
}

That does not affect the API changes and could be done later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will help in avoiding misuse of the staging API --- the users are forced to think what's staging and its difference from macros.

@nicolasstucki nicolasstucki merged commit a3631f8 into scala:master Jan 18, 2021
@nicolasstucki nicolasstucki deleted the rename-staging-toolbox branch January 18, 2021 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants