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

Inner class methods not inlined when compiled separately #4925

Closed
scabug opened this issue Aug 18, 2011 · 6 comments
Closed

Inner class methods not inlined when compiled separately #4925

scabug opened this issue Aug 18, 2011 · 6 comments
Assignees
Labels

Comments

@scabug
Copy link

@scabug scabug commented Aug 18, 2011

// A.scala
class A {
  final class Inner {
    @inline def foo = 7
  }
  def inner = new Inner 
}
// B.scala
class B {
  def baz = {
    val a = new A
    val o = a.inner
    val z = o.foo
    println(z)
  }
}

Compile with:

$ scalac A.scala
$ scalac -optimize B.scala
B.scala:8: warning: Could not inline required method foo because bytecode was not available.
    val z = o.foo
              ^
one warning found

Expected behavior would be to inline foo, as correctly happens when both files are compiled together.

Likely cause:

ICodeReader overrides ClassfileParser.classNameToSymbol but does not replicate the logic that deals with inner classes (i.e. try innerClasses.get(name) object before trying definitions.getClass(name))

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 18, 2011

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 15, 2012

@paulp said:
Surprisingly, this bug seems to be a mirage. It's only due to their being in the empty package, and whether bug or not, empty package classes aren't visible through definitions.getClass. If I put them both in the same package there's no warning and I can see it inlined:

public void baz();
  Code:
   Stack=2, Locals=2, Args_size=1
   0:	new	#7; //class bip/A
   3:	dup
   4:	invokespecial	#11; //Method bip/A."<init>":()V
   7:	astore_1
   8:	aload_1
   9:	invokevirtual	#15; //Method bip/A.inner:()Lbip/A$Inner;
   12:	pop
   13:	getstatic	#21; //Field scala/Predef$.MODULE$:Lscala/Predef$;
   16:	bipush	7
   18:	invokestatic	#27; //Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
   21:	invokevirtual	#31; //Method scala/Predef$.println:(Ljava/lang/Object;)V
   24:	return

So I'm closing this, but if there is still an issue which is being obscured by the empty package thing let me know.

@scabug scabug closed this Jan 15, 2012
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 17, 2012

@TiarkRompf said:
But it should still inline in the given case, don't you think? So I would tend to say it's still a bug, although less severe. (Btw, what happens if they reside in different packages?)

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 17, 2012

@paulp said:
It works if they're in different packages, or if the inlining class is in the empty package. It's only if the callee is in the empty package it fails, because definitions.getClass will not admit to knowledge of empty package classes. It might be easy to fix, I'm just disinclined to work too hard on it since it's essentially cosmetic. (On the other hand I DO want to get trait methods inlining...)

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 17, 2012

@paulp said:
Oh I see, you were right that it's that override, because the superclass has logic to deal specifically with this:

  if (name.pos('.') == name.length)
    definitions.getMember(definitions.EmptyPackageClass, name.toTypeName)
  else
    definitions.getClass(name) // see tickets #2464, #3756
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 17, 2012

@paulp said:
OK, fixed in 2820770bff .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.