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
Feature/inheritdoc and proper usecase comment inheritance/override #70
Conversation
After all, not sure this is the best place to put it
I added you to the scala organization. I have to send this one back to the drawing board as well though.
I spot the whitespace errors with "git diff", which is easier than it sounds because I have it configured to show whitespace differences in reverse video. So introducing new whitespace looks like a giant white blanket attacking the nearby code. |
Use delegation over inheritance
Refchecks - another warnings cleanup
This commit uncomments the disabled `productElementName` method. The benefit of this change is that it becomes possible to pretty-print `Product` instances with field, for example ```scala case class User(name: String, age: Int) def pretty(p: Product): String = p.productElementNames.zip(p.productIterator) .map { case (name, value) => s"$name=$value" } .mkString(p.productPrefix + "(", ", ", ")") toPrettyProduct(User("Susan", 42)) // res0: String = User(name=Susan, age=42) ``` The downside of this change is that it produces more bytecode for each case-class definition. I am not versed enough in reading javap output to determine the exact bytecode size for a given method. I tried the following: ```scala > case class A(a: Int, b: Int, c: Int) > :javap -c A public java.lang.String productElementName(int); Code: 0: iload_1 1: istore_2 2: iload_2 3: tableswitch { // 0 to 2 0: 28 1: 33 2: 38 default: 43 } 28: ldc scala#78 // String a 30: goto 58 33: ldc scala#79 // String b 35: goto 58 38: ldc scala#80 // String c 40: goto 58 43: new scala#67 // class java/lang/IndexOutOfBoundsException 46: dup 47: iload_1 48: invokestatic scala#65 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer; 51: invokevirtual scala#70 // Method java/lang/Object.toString:()Ljava/lang/String; 54: invokespecial scala#73 // Method java/lang/IndexOutOfBoundsException."<init>":(Ljava/lang/String;)V 57: athrow 58: areturn ```
This commit uncomments the disabled `productElementName` method. The benefit of this change is that it becomes possible to pretty-print `Product` instances with field, for example ```scala case class User(name: String, age: Int) def pretty(p: Product): String = p.productElementNames.zip(p.productIterator) .map { case (name, value) => s"$name=$value" } .mkString(p.productPrefix + "(", ", ", ")") toPrettyProduct(User("Susan", 42)) // res0: String = User(name=Susan, age=42) ``` The downside of this change is that it produces more bytecode for each case-class definition. I am not versed enough in reading javap output to determine the exact bytecode size for a given method. I tried the following: ```scala > case class A(a: Int, b: Int, c: Int) > :javap -c A public java.lang.String productElementName(int); Code: 0: iload_1 1: istore_2 2: iload_2 3: tableswitch { // 0 to 2 0: 28 1: 33 2: 38 default: 43 } 28: ldc scala#78 // String a 30: goto 58 33: ldc scala#79 // String b 35: goto 58 38: ldc scala#80 // String c 40: goto 58 43: new scala#67 // class java/lang/IndexOutOfBoundsException 46: dup 47: iload_1 48: invokestatic scala#65 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer; 51: invokevirtual scala#70 // Method java/lang/Object.toString:()Ljava/lang/String; 54: invokespecial scala#73 // Method java/lang/IndexOutOfBoundsException."<init>":(Ljava/lang/String;)V 57: athrow 58: areturn ```
This commit uncomments the disabled `productElementName` method. The benefit of this change is that it becomes possible to pretty-print `Product` instances with field, for example ```scala case class User(name: String, age: Int) def pretty(p: Product): String = p.productElementNames.zip(p.productIterator) .map { case (name, value) => s"$name=$value" } .mkString(p.productPrefix + "(", ", ", ")") toPrettyProduct(User("Susan", 42)) // res0: String = User(name=Susan, age=42) ``` The downside of this change is that it produces more bytecode for each case-class definition. Running `:javacp -c` for a case class with three fields yields the following results ```scala > case class A(a: Int, b: Int, c: Int) > :javap -c A public java.lang.String productElementName(int); Code: 0: iload_1 1: istore_2 2: iload_2 3: tableswitch { // 0 to 2 0: 28 1: 33 2: 38 default: 43 } 28: ldc scala#78 // String a 30: goto 58 33: ldc scala#79 // String b 35: goto 58 38: ldc scala#80 // String c 40: goto 58 43: new scala#67 // class java/lang/IndexOutOfBoundsException 46: dup 47: iload_1 48: invokestatic scala#65 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer; 51: invokevirtual scala#70 // Method java/lang/Object.toString:()Ljava/lang/String; 54: invokespecial scala#73 // Method java/lang/IndexOutOfBoundsException."<init>":(Ljava/lang/String;)V 57: athrow 58: areturn ``` Thanks to Adriaan's help, the estimated cost per `productElementName` appears to be fixed 56 bytes and then 10 bytes for each field with the following breakdown: * 3 bytes for the [string info](https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4.3) (the actual characters are already in the constant pool) * 4 bytes for the tableswitch entry * 2 bytes for the ldc to load the string * 1 byte for areturn In my opinion, the bytecode cost is acceptably low thanks to the fact that field name literals are already available in the constant pool.
This commit adds two methods to the `scala.Product` trait: ```scala trait Product { /** Returns the field name of element at index n */ def productElementName(n: Int): String /** Returns field names of this product. Must have same length as productIterator */ def productElementNames: Iterator[String] } ``` Both methods have a default implementation which returns the empty string for all field names. This commit then changes the code-generation for case classes to synthesize a `productElementName` method with actual class field names. The benefit of this change is that it becomes possible to pretty-print case classes with field names, for example ```scala case class User(name: String, age: Int) def toPrettyString(p: Product): String = p.productElementNames.zip(p.productIterator) .map { case (name, value) => s"$name=$value" } .mkString(p.productPrefix + "(", ", ", ")") toPrettyString(User("Susan", 42)) // res0: String = User(name=Susan, age=42) ``` The downside of this change is that it produces more bytecode for each case-class definition. Running `:javacp -c` for a case class with three fields yields the following results ```scala > case class A(a: Int, b: Int, c: Int) > :javap -c A public java.lang.String productElementName(int); Code: 0: iload_1 1: istore_2 2: iload_2 3: tableswitch { // 0 to 2 0: 28 1: 33 2: 38 default: 43 } 28: ldc scala#78 // String a 30: goto 58 33: ldc scala#79 // String b 35: goto 58 38: ldc scala#80 // String c 40: goto 58 43: new scala#67 // class java/lang/IndexOutOfBoundsException 46: dup 47: iload_1 48: invokestatic scala#65 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer; 51: invokevirtual scala#70 // Method java/lang/Object.toString:()Ljava/lang/String; 54: invokespecial scala#73 // Method java/lang/IndexOutOfBoundsException."<init>":(Ljava/lang/String;)V 57: athrow 58: areturn ``` Thanks to Adriaan's help, the estimated cost per `productElementName` appears to be fixed 56 bytes and then 10 bytes for each field with the following breakdown: * 3 bytes for the [string info](https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4.3) (the actual characters are already in the constant pool) * 4 bytes for the tableswitch entry * 2 bytes for the ldc to load the string * 1 byte for areturn In my opinion, the bytecode cost is acceptably low thanks to the fact that field name literals are already available in the constant pool.
It didn't merge properly into master, because git was skipping one commit along the way (no idea why git was simply skipping over it). Rebased on the latest master and everything works fine now.
@paulp Do you use a tool to check for whitespace? If so, please let me know so I get a better pull request acceptance rate :p And btw, no, I can't see whether the pull request is not merging well, I'm not part of the scala organization :(