Skip to content

Conversation

@retronym
Copy link
Member

@retronym retronym commented Jul 25, 2016

  • Avoid calling NoSymbol.owner when checking whether we're
    dealing with a case class constructor pattern or a general
    extractor. Tested manually with the test case in the ticket,
    no more output is produced under -Xdev.
  • Be more conservative about the conversion to a case class
    pattern: rather than looking just at the type of the pattern
    tree, also look at the tree itself to ensure its safe to
    elide. This change is analagous to SI-4859, which restricted
    rewrites of case apply calls to case constructors.

I've manually tested that case class patterns are still efficiently
translated:

object Test {
  def main(args: Array[String]) {
    Some(1) match { case Some(x) => }
  }
}

% qscalac -Xprint:patmat sandbox/test.scala
[[syntax trees at end of                    patmat]] // test.scala
package <empty> {
  object Test extends scala.AnyRef {
    def <init>(): Test.type = {
      Test.super.<init>();
      ()
    };
    def main(args: Array[String]): Unit = {
      case <synthetic> val x1: Some[Int] = scala.Some.apply[Int](1);
      case4(){
        if (x1.ne(null))
          matchEnd3(())
        else
          case5()
      };
      case5(){
        matchEnd3(throw new MatchError(x1))
      };
      matchEnd3(x: Unit){
        x
      }
    }
  }
}

  - Avoid calling NoSymbol.owner when checking whether we're
    dealing with a case class constructor pattern or a general
    extractor. Tested manually with the test case in the ticket,
    no more output is produced under `-Xdev`.
  - Be more conservative about the conversion to a case class
    pattern: rather than looking just at the type of the pattern
    tree, also look at the tree itself to ensure its safe to
    elide. This change is analagous to SI-4859, which restricted
    rewrites of case apply calls to case constructors.

I've manually tested that case class patterns are still efficiently
translated:

```
object Test {
  def main(args: Array[String]) {
    Some(1) match { case Some(x) => }
  }
}

```

```
% qscalac -Xprint:patmat sandbox/test.scala
[[syntax trees at end of                    patmat]] // test.scala
package <empty> {
  object Test extends scala.AnyRef {
    def <init>(): Test.type = {
      Test.super.<init>();
      ()
    };
    def main(args: Array[String]): Unit = {
      case <synthetic> val x1: Some[Int] = scala.Some.apply[Int](1);
      case4(){
        if (x1.ne(null))
          matchEnd3(())
        else
          case5()
      };
      case5(){
        matchEnd3(throw new MatchError(x1))
      };
      matchEnd3(x: Unit){
        x
      }
    }
  }
}
```
@retronym retronym added this to the 2.12.0-RC1 milestone Jul 25, 2016
@retronym
Copy link
Member Author

Review by @adriaanm

Spun out of a fix in #5263.

@adriaanm adriaanm self-assigned this Jul 26, 2016
@adriaanm
Copy link
Contributor

LGTM

@adriaanm adriaanm merged commit efb74b7 into scala:2.12.x Jul 26, 2016
@adriaanm adriaanm added the 2.12 label Oct 29, 2016
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.

3 participants