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

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

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 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

docs/docs/reference/metaprogramming/staging.md Outdated Show resolved Hide resolved
@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).
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.

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.

None yet

2 participants