Skip to content

Backend backport2 #124

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 8 commits into from
May 7, 2014
Merged

Backend backport2 #124

merged 8 commits into from
May 7, 2014

Conversation

DarkDimius
Copy link
Contributor

The most important bit is fix for erasure, that was emitting such code:

def tailRec(x: Int): Unit = {
  if scala.Int.unbox(x).<(0) then {
    x
    ()
  } else {
    println(scala.Int.box(x))
    C.tailRec(scala.Int.unbox(x).-(1))
  }
}

@DarkDimius
Copy link
Contributor Author

@odersky lets get this merged, it fixes several issues that trigger failures in checker.

@DarkDimius
Copy link
Contributor Author

@odersky @gzm0 please review

@DarkDimius DarkDimius mentioned this pull request May 3, 2014
@gzm0
Copy link
Contributor

gzm0 commented May 5, 2014

LGTM

@odersky
Copy link
Contributor

odersky commented May 5, 2014

I'm OK with the rationale you have given for flags naming.

@DarkDimius
Copy link
Contributor Author

ping @odersky
Please have a look. I've incorporated all comments.

@DarkDimius DarkDimius mentioned this pull request May 6, 2014
def isJavaMainMethod(sym: Symbol)(implicit ctx: Context) = {
val dn = defn
(sym.name == nme.main) && (sym.info match {
case t@MethodType(_, dn.ArrayType(el) :: Nil) => el =:= defn.StringType && t.resultType.typeSymbol == defn.UnitClass
Copy link
Contributor

Choose a reason for hiding this comment

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

t.resultType isRef dn.Unit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I understand correctly that this is just shorter way to do same test?
Or are they actually different?

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case it's just shorter, but in general isRef is more robust. I flagged it because it's important to be consistent everywhere. Otherwise the next person will choose one randomly depending on what idiom saw previously.

@odersky
Copy link
Contributor

odersky commented May 6, 2014

Otherwise, LGTM

lazy val Array_apply = ctx.requiredMethod(ArrayClass, nme.apply)
lazy val Array_update = ctx.requiredMethod(ArrayClass, nme.update)
lazy val Array_length = ctx.requiredMethod(ArrayClass, nme.length)
lazy val Array_clone = ctx.requiredMethod(ArrayClass, nme.clone_)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it matters in Dotty, but in scalac these should be def-s, not vals. Why? Subsequent runs of the compiler might recompile Array (this happens if you navigate to its sources in Scala IDE, for example), which would result in new symbols for its members.

That's why I split out RunDefinitions in scalac, so we could have correctness and speed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For Dotty definitions are part of context and I would say that in case discarding is required, the better approach would be to discard context itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed we should do the same thing. (Keep them as lazyvals but break them
out into a new class) It's planned by has not been done yet. - Martin

On Tue, May 6, 2014 at 4:11 PM, Jason Zaugg [email protected]:

In src/dotty/tools/dotc/core/Definitions.scala:

@@ -168,6 +170,10 @@ class Definitions {
List(AnyClass.typeRef), EmptyScope)
lazy val SeqClass: ClassSymbol = ctx.requiredClass("scala.collection.Seq")
lazy val ArrayClass: ClassSymbol = ctx.requiredClass("scala.Array")

  • lazy val Array_apply = ctx.requiredMethod(ArrayClass, nme.apply)
  • lazy val Array_update = ctx.requiredMethod(ArrayClass, nme.update)
  • lazy val Array_length = ctx.requiredMethod(ArrayClass, nme.length)
  • lazy val Array_clone = ctx.requiredMethod(ArrayClass, nme.clone_)

Not sure if it matters in Dotty, but in scalac these should be def-s, not
vals. Why? Subsequent runs of the compiler might recompile Array (this
happens if you navigate to its sources in Scala IDE, for example), which
would result in new symbols for its members.

That's why I split out RunDefinitions in scalac, so we could have
correctness and speed.


Reply to this email directly or view it on GitHubhttps://github.com//pull/124/files#r12325734
.

Martin Odersky

EPFL

JOIN US. REGISTER TODAY! http://www.scaladays.org/
Scala http://www.scaladays.org/
Days http://www.scaladays.org/
June 16th-18th, http://www.scaladays.org/
Berlin http://www.scaladays.org/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Martin, why do you need a new class for this? Isn't Definitions itself a class that can be discarded with context holding it.

DarkDimius added 7 commits May 6, 2014 16:18
Collect entry points for backend.
Previously this was done by cleanup.
TermRef's for primitive types are of primitive-type.
Conflicts:
	src/dotty/tools/dotc/backend/jvm/BCodeBodyBuilder.scala
	src/dotty/tools/dotc/core/Definitions.scala
DarkDimius added a commit that referenced this pull request May 7, 2014
@DarkDimius DarkDimius merged commit c5c400c into scala:master May 7, 2014
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.

4 participants