Change to Constant.escapedStringValue

Grzegorz,

That patch reminded me of other improvements, that you can find under

def unparseConstant(vc: Constant): String

at

http://lampsvn.epfl.ch/trac/scala/browser/scala-experimental/trunk/jdk2i...

That code is part of a work-in-progress "unparser" that might also
contain other code snippets of use. The unparser in question is part of an
API migration tool ("jdk2ikvm") but also intended to be used independently,
as
described at:

http://lamp.epfl.ch/~magarcia/ScalaCompilerCornerReloaded/2010Q4/Unparsi...

The unparser as of now gets tripped when printing some type references,
I have to take a look at how Scaladoc does that to get it right. Other than
that,
it's doing a fine job of serializing (possibly modified) Scala ASTs into
.scala files.

Miguel

2011/1/1 Miguel Garcia :
>
> Grzegorz,
>
> That patch reminded me of other improvements, that you can find under
>
> def unparseConstant(vc: Constant): String
>
> at
>
> http://lampsvn.epfl.ch/trac/scala/browser/scala-experimental/trunk/jdk2i...
>
> That code is part of a work-in-progress "unparser" that might also
> contain other code snippets of use. The unparser in question is part of an
> API migration tool ("jdk2ikvm") but also intended to be used independently,
> as
> described at:
>
> http://lamp.epfl.ch/~magarcia/ScalaCompilerCornerReloaded/2010Q4/Unparsi...
>
> The unparser as of now gets tripped when printing some type references,
> I have to take a look at how Scaladoc does that to get it right. Other than
> that,
> it's doing a fine job of serializing (possibly modified) Scala ASTs into
> .scala files.

Thanks Miguel for your response! Although it doesn't address my
question directly it's really stuff that you pointed me at. I'll try
to go through pdf you prepared, soon. Do I understand you that
unparsing functionality is ready to use so I can think of merging your
code into my fork of scalac? Would it work fine with ASTs after
cleanup phase?

It sounds like something that would help with my experiments with
jribble backend.

On Sat, Jan 01, 2011 at 08:47:06PM +0100, Grzegorz Kossakowski wrote:
> Do you think it's a good idea to apply such a change?

I do. It's better to write it like this though. I'll check it in as
soon with some near future batch.

@switch def escapedChar(ch: Char) = ch match {
case '\b' => "\\b"
case '\t' => "\\t"
case '\n' => "\\n"
case '\f' => "\\f"
case '\r' => "\\r"
case '"' => "\\\""
case '\'' => "\\\'"
case '\\' => "\\\\"
case _ => "" + ch
}

Don't need to allocate all those options:

public java.lang.String escapedChar(char);
Code:
Stack=2, Locals=3, Args_size=2
0: iload_1
1: istore_2
2: iload_2
3: lookupswitch{ //8
8: 144;
9: 138;
10: 132;
12: 126;
13: 120;
34: 114;
39: 108;
92: 102;
default: 76 }
76: new #585858; //class scala/collection/mutable/StringBuilder
79: dup
80: invokespecial #555555; //Method scala/collection/mutable/StringBuilder."":()V
83: ldc_w #2a2a2a; //String
86: invokevirtual #606060; //Method scala/collection/mutable/StringBuilder.append:(Ljava/lang/Object;)Lscala/collection/mutable/StringBuilder;
89: iload_1
90: invokestatic #5f5f5f; //Method scala/runtime/BoxesRunTime.boxToCharacter:(C)Ljava/lang/Character;
93: invokevirtual #606060; //Method scala/collection/mutable/StringBuilder.append:(Ljava/lang/Object;)Lscala/collection/mutable/StringBuilder;
96: invokevirtual #636363; //Method scala/collection/mutable/StringBuilder.toString:()Ljava/lang/String;
99: goto 147
102: ldc_w #252525; //String \\
105: goto 147
108: ldc_w #282828; //String \'
111: goto 147
114: ldc_w #2c2c2c; //String \\"
117: goto 147
120: ldc_w #303030; //String \r
123: goto 147
126: ldc_w #343434; //String \f
129: goto 147
132: ldc_w #2f2f2f; //String \n
135: goto 147
138: ldc_w #333333; //String \t
141: goto 147
144: ldc_w #363636; //String \b
147: areturn

2011/1/1 Grzegorz Kossakowski :
> 2011/1/1 Miguel Garcia :
>>
>> Grzegorz,
>>
>> That patch reminded me of other improvements, that you can find under
>>
>> def unparseConstant(vc: Constant): String
>>
>> at
>>
>> http://lampsvn.epfl.ch/trac/scala/browser/scala-experimental/trunk/jdk2i...
>>
>> That code is part of a work-in-progress "unparser" that might also
>> contain other code snippets of use. The unparser in question is part of an
>> API migration tool ("jdk2ikvm") but also intended to be used independently,
>> as
>> described at:
>>
>> http://lamp.epfl.ch/~magarcia/ScalaCompilerCornerReloaded/2010Q4/Unparsi...
>>
>> The unparser as of now gets tripped when printing some type references,
>> I have to take a look at how Scaladoc does that to get it right. Other than
>> that,
>> it's doing a fine job of serializing (possibly modified) Scala ASTs into
>> .scala files.
>
> Thanks Miguel for your response! Although it doesn't address my
> question directly it's really stuff that you pointed me at. I'll try
> to go through pdf you prepared, soon. Do I understand you that
> unparsing functionality is ready to use so I can think of merging your
> code into my fork of scalac? Would it work fine with ASTs after
> cleanup phase?

Oh, I just noticed 5 TODO chapter. What a pity. Anyway, I'll keep my
eye on your work.

2011/1/1 Paul Phillips :
> On Sat, Jan 01, 2011 at 08:47:06PM +0100, Grzegorz Kossakowski wrote:
>> Do you think it's a good idea to apply such a change?
>
> I do.  It's better to write it like this though.  I'll check it in as
> soon with some near future batch.
>
>    @switch def escapedChar(ch: Char) = ch match {
>      case '\b' => "\\b"
>      case '\t' => "\\t"
>      case '\n' => "\\n"
>      case '\f' => "\\f"
>      case '\r' => "\\r"
>      case '"'  => "\\\""
>      case '\'' => "\\\'"
>      case '\\' => "\\\\"
>      case _    => "" + ch
>    }
>
> Don't need to allocate all those options:
[...]

Thanks Paul, it's a very good suggestion. I'm still learning how to
write code not only correct but efficient at the same time.

On Sat, Jan 1, 2011 at 8:28 PM, Paul Phillips wrote:
> I do.  It's better to write it like this though.  I'll check it in as
> soon with some near future batch.

This is certainly better, but it can be improved further.

>      case _    => "" + ch

Replace by:

case _ => String.valueOf(ch)

And the StringBuilder allocation disappears:

public java.lang.String escapedChar(char);
Code:
Stack=1, Locals=2, Args_size=2
0: iload_1
1: lookupswitch{ //8
8: 118;
9: 113;
10: 108;
12: 103;
13: 98;
34: 93;
39: 88;
92: 83;
default: 76 }
76: iload_1
77: invokestatic #11; //Method java/lang/String.valueOf:(C)Ljava/lang/String;
80: goto 120
83: ldc #13; //String \\
85: goto 120
88: ldc #15; //String \'
90: goto 120
93: ldc #17; //String \\"
95: goto 120
98: ldc #19; //String \r
100: goto 120
103: ldc #21; //String \f
105: goto 120
108: ldc #23; //String \n
110: goto 120
113: ldc #25; //String \t
115: goto 120
118: ldc #27; //String \b
120: areturn

Best,
Ismael

Interesting:

2950            /**
2951             * Returns the string representation of the <code>char</code>
2952             * argument.
2953             *
2954             * @param   c   a <code>char</code>.
2955             * @return  a string of length <code>1</code> containing
2956             *          as its single character the argument <code>c</code>.
2957             */
2958            public static String valueOf(char c) {
2959                char data[] = { c };
2960                return new String(0, 1, data);
2961            }

On Sat, Jan 1, 2011 at 10:20 PM, Ismael Juma <mlists@juma.me.uk> wrote:

On Sat, Jan 1, 2011 at 8:28 PM, Paul Phillips <paulp@improving.org> wrote:
> I do.  It's better to write it like this though.  I'll check it in as
> soon with some near future batch.

This is certainly better, but it can be improved further.

>      case _    => "" + ch

Replace by:

case _ => String.valueOf(ch)

And the StringBuilder allocation disappears:

public java.lang.String escapedChar(char);
 Code:
  Stack=1, Locals=2, Args_size=2
  0:   iload_1
  1:   lookupswitch{ //8
               8: 118;
               9: 113;
               10: 108;
               12: 103;
               13: 98;
               34: 93;
               39: 88;
               92: 83;
               default: 76 }
  76:  iload_1
  77:  invokestatic    #11; //Method java/lang/String.valueOf:(C)Ljava/lang/String;
  80:  goto    120
  83:  ldc     #13; //String \\
  85:  goto    120
  88:  ldc     #15; //String \'
  90:  goto    120
  93:  ldc     #17; //String \\"
  95:  goto    120
  98:  ldc     #19; //String \r
  100: goto    120
  103: ldc     #21; //String \f
  105: goto    120
  108: ldc     #23; //String \n
  110: goto    120
  113: ldc     #25; //String \t
  115: goto    120
  118: ldc     #27; //String \b
  120: areturn

Best,
Ismael

--
Viktor Klang,
Code Connoisseur
Work:   Scalable Solutions
Code:   github.com/viktorklang
Follow: twitter.com/viktorklang
Read:   klangism.tumblr.com

On Sat, Jan 01, 2011 at 09:20:43PM +0000, Ismael Juma wrote:
> This is certainly better, but it can be improved further.
>
> And the StringBuilder allocation disappears:

Oh, that's excellent, thanks.

I mean, don't tell me how to optimize, you glunk! Do I come to your
house and tell you how to write code so beautiful people like to read it
when on holiday? (Have to take care of my reputation, you understand.)

Actually on this point: I have dropped all my projects mid-juggle to try
to make the compiler faster. I have made some progress:

http://www.scala-lang.org/node/359/trunk!Compilation time - quick_comp

I drove the bus for the recent trip from 285 to 257. I have a few more
things cooking. Grzegorz, if you or anyone else have compiler matters
for which you would like my attention, a great way to get it right now
is to help me pinpoint the bottlenecks in scalac. I have a lot of stuff
cooking but I know there is plenty left to find.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/01/11 05:47, Grzegorz Kossakowski wrote:
> Hello,
>
> I'd like someone familiar with Constants.scala functionality. I
> changed implementation of escapedStringValue to escape more
> characters. You can see my patch here:
>
> http://review.source.gogoego.com/#patch,sidebyside,1528,1,src/library/sc...
>
> I needed this functionality because I rely on it in my jribble backed
> which is part of scalagwt project. More specifically, I rely on
> TreePrinters that make calls to escapedStringValue method.
>
> Do you think it's a good idea to apply such a change?
>
Side note re lines 214-217

if(p)
buf.append(x)
else
buf.append(y)

is better written (move the side-effect to the outside):

buf.append(if(p) x else y)

(there is also the issue of using String.+ inside the for while also
StringBuilder.append)

but then that method is better written without StringBuilder altogether:

def escape(text: String): String) = {
val escapes = ...
text flatMap (c => if(c.isControl)
"\\0"+toOctalString(c.asInstanceOf[Int]) else
"\\0"+toOctalString(c.asInstanceOf[Int]))
}

On Sat, Jan 1, 2011 at 9:39 PM, Paul Phillips wrote:
> I mean, don't tell me how to optimize, you glunk! Do I come to your
> house and tell you how to write code so beautiful people like to read it
> when on holiday? (Have to take care of my reputation, you understand.)

:)

> Actually on this point: I have dropped all my projects mid-juggle to try
> to make the compiler faster.  I have made some progress:

I had noticed the commits. Thanks for doing this, it's appreciated!

>  http://www.scala-lang.org/node/359/trunk!Compilation time - quick_comp
>
> I drove the bus for the recent trip from 285 to 257.

Interesting. I notice that we were at 205ms a few months ago and there
was a consistent slowdown all the way to 285ms before you went to work
on bringing it down again. I guess there is no easy way to fix the
performance regressions introduced during that period?

Best,
Ismael

On Sun, Jan 02, 2011 at 05:31:10PM +0000, Ismael Juma wrote:
> Interesting. I notice that we were at 205ms a few months ago and there
> was a consistent slowdown all the way to 285ms before you went to work
> on bringing it down again. I guess there is no easy way to fix the
> performance regressions introduced during that period?

As yet nobody knows what they are. Rather than spend too much time
trying to pinpoint them I'm assuming nothing and experimentally
investigating what things are slow. But if anyone wanted to figure out
why exactly we went from 205 to 285 that would fall under the category
of "things which would help me out."