Fix #113 and apply several other fixes #221
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These helpers are only used in `Dependency` for now, but I feel that they could become handy in other Zinc scalac phases and remove the overplaty way of guarding against inexistent or useless symbols and types.
Instead of gathering all symbols in a set and using `foreach`, reuse a foreach symbol traverser that will go through all the symbols of a type and execute an operation on them. This finishes up similar work done by @romanowski. Apparently, this instance of `symbolsInType` was forgotten. As this function is not useful anymore, it is removed for the posterity.
The name `foreachSymbolInType` is misleading in the sense that gives the impression to the reader that a certain operation will be applied to every type symbol in a type, whereas it only does so for symbol that don't correspond to packages. This commit renames this method to a longer but more concise version of this function. I haven't skimped on the length of the name because, IMO, helpers should prefer correctness over shortness.
This commit is an aesthetic change to the code of `ExtractUsedNames` in the following ways: * It clarifies the purpose of the `termSymbol` attached to `Import` nodes and removes (now) unnecessary comments from the source. * It adds curly braces around a long method for readability purposes. * It removes `tpnme` which was introduced for 2.8 Scala support. Zinc does not anymore offer support for 2.8.x and 2.9.x, hence the removal. * It moves `emptyName` to `GlobalHelpers` under `isEmptyName` and instead of guarding against `NoSymbol` it reuses the helper `ignoredSymbol`.
`SafeLazy` has been traditionally implemented in `zincApiInfo` because it is part of the sbt API and is accessible to all the subprojects that depend on it. Before this commit, `SafeLazy` was a runtime dependency (using reflection) of the compiler bridge. In this regard, Zinc was assuming that the sbt API was accessible at runtime and therefore invoked it to use an implementation of lazy that would remove references to the thunks once they've been forced. This was done to free memory as soon as possible since those thunks usually depend on classes of compiler internals and would not be GC'ed otherwise. However, the compiler bridge is not supposed to depend on sbt APIs since its code is compiled by the Scala compiler that the user picks in SBT. Its only dependency is the compiler interface, which is implemented in Java and compiled beforehand with javac. This commit removes the runtime dependency of the compiler bridge to the sbt API and avoids the method invocations using reflection. This was done for the following reasons: * Simplicity. It is not obvious why `SafeLazy` is invoked reflectively. See sbt#113. * Performance. Even though the JVM should make this simple use of reflection fast, there's a very small overhead of using reflection in the compiler bridge because `lzy` is (most likely) hot. The fix consists of a Java implementation of `SafeLazy` that uses the non-thread-safe lazy val implementation described [here](http://docs.scala-lang.org/sips/pending/improved-lazy-val-initialization.html). It is complemented with a proxy written in Scala that will create an indirection layer for things like by-name and strict evaluation. This implementation of lazy val assumes that `SafeLazy` will never be called asynchronously. If this is the case, it's up to the Zinc maintainer to make sure that safe publishing is implemented at the call-site or to change the implementation to avoid races and uninitialized fields.
|
LGTM Did you run some benchmarks? I am really interested how much we gain from dropping reflection. |
|
It probably doesn't make a difference because the JVM should be able to do de-reflection optimizations since the method called via reflection is always the same: http://stackoverflow.com/a/28809546/348497 |
|
@romanowski Correct @smarter, I explain it here in this commit message. |
Future error reporting would be more sophisticated because ideally we want to report concise error messages with good contextual information. This commit takes the first step by putting common error messages in an object `Feedback` stored into `GlobalHelpers`. Temporary error messages have not been added since they will be removed in future commits (things like `super` not being handled in `ExtractAPI`, for instance).
Let's make sure this class is not extended in future versions by inheritance.
eed3si9n
approved these changes
Feb 7, 2017
eed3si9n
pushed a commit
to eed3si9n/scala
that referenced
this pull request
May 14, 2019
This commit introduces changes to Scala 2.10 sources from the following PRs: * sbt/zinc#225 * sbt/zinc#216 * sbt/zinc#206 * sbt/zinc#221 It also removes a stub for 2.8 compatibility in `DelegatingReporter`. Support for Scala 2.8 compatibility is not already maintained it.
lrytz
pushed a commit
to lrytz/scala
that referenced
this pull request
Nov 5, 2019
This commit introduces changes to Scala 2.10 sources from the following PRs: * sbt/zinc#225 * sbt/zinc#216 * sbt/zinc#206 * sbt/zinc#221 It also removes a stub for 2.8 compatibility in `DelegatingReporter`. Support for Scala 2.8 compatibility is not already maintained it. Rewritten from sbt/zinc@5d46c1b
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes one issue, cleans up a little bit of Zinc code, adds more helpers
and makes some performance fixes to the code.
Please, for full information on the issues fixed, have a look at the commit
messages.