-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Rename scala.quoted.staging.{Toolbox => Compiler} #11129
Conversation
1291e09
to
cbc497d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, LGTM
44d083d
to
3c215db
Compare
@liufengyun could you double-check that I did not miss something while renaming from |
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).
3c215db
to
1e0475c
Compare
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) |
There was a problem hiding this comment.
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.{...}
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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).