Better fix for FrozenString. String-fed FrozenStrings should just pas…

…s through to String Literal rather than get reconstructed from generate bytelist. Frozen string optimization did not propagate coderange to FrozenString. XStrNode when constructed from StrNode did not propagate coderange.
jruby · Mar 12, 2015 · 18d9c88 · 18d9c88 · scrhartley · Mar 13, 2015
1 parent f7622b2
commit 18d9c88
Showing 6 changed files with 43 additions and 26 deletions.
diff --git a/core/src/main/java/org/jruby/ast/XStrNode.java b/core/src/main/java/org/jruby/ast/XStrNode.java
@@ -42,11 +42,13 @@
  */
 public class XStrNode extends Node implements ILiteralNode {
     private final ByteList value;
+    private int coderange;
 
-    public XStrNode(ISourcePosition position, ByteList value) {
+    public XStrNode(ISourcePosition position, ByteList value, int coderange) {
         // FIXME: Shouldn't this have codeRange like StrNode?
         super(position, false);
         this.value = (value == null ? ByteList.create("") : value);
+        this.coderange = coderange;
     }
 
     public NodeType getNodeType() {
@@ -68,6 +70,10 @@ public <T> T accept(NodeVisitor<T> iVisitor) {
     public ByteList getValue() {
         return value;
     }
+
+    public int getCodeRange() {
+        return coderange;
+    }
 
     public List<Node> childNodes() {
         return EMPTY_LIST;

diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -1004,10 +1004,10 @@ public Operand buildCall(CallNode callNode) {
         Node callArgsNode = callNode.getArgsNode();
         Node receiverNode = callNode.getReceiverNode();
 
-        // check for "string".freeze
+        // Frozen string optimization: check for "string".freeze
         if (receiverNode instanceof StrNode && callNode.getName().equals("freeze")) {
-            // frozen string optimization
-            return new FrozenString(((StrNode)receiverNode).getValue(), StringSupport.CR_7BIT);
+            StrNode asString = (StrNode) receiverNode;
+            return new FrozenString(asString.getValue(), asString.getCodeRange());
         }
 
         // Though you might be tempted to move this build into the CallInstr as:
@@ -3399,7 +3399,7 @@ public Operand buildWhile(final WhileNode whileNode) {
     }
 
     public Operand buildXStr(XStrNode node) {
-        return addResultInstr(new BacktickInstr(createTemporaryVariable(), new Operand[] { new StringLiteral(node.getValue())}));
+        return addResultInstr(new BacktickInstr(createTemporaryVariable(), new Operand[] { new StringLiteral(node.getValue(), node.getCodeRange())}));
     }
 
     public Operand buildYield(YieldNode node) {

diff --git a/core/src/main/java/org/jruby/ir/operands/FrozenString.java b/core/src/main/java/org/jruby/ir/operands/FrozenString.java
@@ -8,7 +8,6 @@
 import org.jruby.runtime.ThreadContext;
 import org.jruby.runtime.builtin.IRubyObject;
 import org.jruby.util.ByteList;
-import org.jruby.util.StringSupport;
 
 /**
  * Represents a literal string value.
@@ -18,12 +17,18 @@
  * This is not like a Java string.
  */
 public class FrozenString extends StringLiteral {
+    /**
+     * Used by persistence and by .freeze optimization
+     */
     public FrozenString(ByteList byteList, int cr) {
         super(OperandType.FROZEN_STRING, byteList, cr);
     }
 
+    /**
+     * IRBuild.buildGetDefinition returns a frozen string and this is for all intern'd Java strings.
+     */
     public FrozenString(String s) {
-        this(ByteList.create(s), StringSupport.CR_7BIT);
+        super(s);
     }
 
     @Override

diff --git a/core/src/main/java/org/jruby/ir/operands/StringLiteral.java b/core/src/main/java/org/jruby/ir/operands/StringLiteral.java
@@ -32,27 +32,31 @@ public class StringLiteral extends Operand {
     final public String   string;
     final public int      coderange;
 
-    public StringLiteral(ByteList val) {
-        this(OperandType.STRING_LITERAL, val, StringSupport.CR_7BIT);
-    }
-
     public StringLiteral(ByteList val, int coderange) {
         this(OperandType.STRING_LITERAL, val, coderange);
     }
 
     protected StringLiteral(OperandType type, ByteList val, int coderange) {
+        this(type, internedStringFromByteList(val), val, coderange);
+
+    }
+
+    protected StringLiteral(OperandType type, String string, ByteList bytelist, int coderange) {
         super(type);
 
-        bytelist = val;
+        this.bytelist = bytelist;
         this.coderange = coderange;
-        String stringTemp;
+        this.string = string;
+    }
+
+    // If Encoding has an instance of a Charset can it ever raise unsupportedcharsetexception? because this
+    // helper called copes with charset == null...
+    private static String internedStringFromByteList(ByteList val) {
         try {
-            stringTemp = Helpers.byteListToString(bytelist);
+            return Helpers.byteListToString(val).intern();
         } catch (UnsupportedCharsetException e) {
-            stringTemp = bytelist.toString();
+            return val.toString().intern();
         }
-        // FIXME:
-        string = stringTemp.intern();
     }
 
     public StringLiteral(String s) {

diff --git a/core/src/main/java/org/jruby/parser/RubyParser.java b/core/src/main/java/org/jruby/parser/RubyParser.java
@@ -128,7 +128,8 @@
 import org.jruby.util.ByteList;
 import org.jruby.util.KeyValuePair;
 import org.jruby.util.cli.Options;
-
+import org.jruby.util.StringSupport;
+
 public class RubyParser {
     protected ParserSupport support;
     protected RubyLexer lexer;
@@ -148,7 +149,7 @@ public void setWarnings(IRubyWarnings warnings) {
         support.setWarnings(warnings);
         lexer.setWarnings(warnings);
     }
-					// line 152 "-"
+					// line 153 "-"
   // %token constants
   public static final int kCLASS = 257;
   public static final int kMODULE = 258;
@@ -4277,9 +4278,9 @@ public Object yyparse (RubyLexer yyLex) throws java.io.IOException {
                     ISourcePosition position = support.getPosition(((Node)yyVals[-1+yyTop]));
 
                     if (((Node)yyVals[-1+yyTop]) == null) {
-                        yyVal = new XStrNode(position, null);
+                        yyVal = new XStrNode(position, null, StringSupport.CR_7BIT);
                     } else if (((Node)yyVals[-1+yyTop]) instanceof StrNode) {
-                        yyVal = new XStrNode(position, (ByteList) ((StrNode)yyVals[-1+yyTop]).getValue().clone());
+                        yyVal = new XStrNode(position, (ByteList) ((StrNode)yyVals[-1+yyTop]).getValue().clone(), ((StrNode)yyVals[-1+yyTop]).getCodeRange());
                     } else if (((Node)yyVals[-1+yyTop]) instanceof DStrNode) {
                         yyVal = new DXStrNode(position, ((DStrNode)yyVals[-1+yyTop]));
 
@@ -5245,7 +5246,7 @@ public Object yyparse (RubyLexer yyLex) throws java.io.IOException {
   }
 };
 }
-					// line 2510 "RubyParser.y"
+					// line 2511 "RubyParser.y"
 
     /** The parse method use an lexer stream and parse it to an AST node 
      * structure
@@ -5264,4 +5265,4 @@ public RubyParserResult parse(ParserConfiguration configuration, LexerSource sou
         return support.getResult();
     }
 }
-					// line 9793 "-"
+					// line 9794 "-"
diff --git a/core/src/main/java/org/jruby/parser/RubyParser.y b/core/src/main/java/org/jruby/parser/RubyParser.y
@@ -125,7 +125,8 @@ import org.jruby.lexer.yacc.SyntaxException.PID;
 import org.jruby.util.ByteList;
 import org.jruby.util.KeyValuePair;
 import org.jruby.util.cli.Options;
-
+import org.jruby.util.StringSupport;
+
 public class RubyParser {
     protected ParserSupport support;
     protected RubyLexer lexer;
@@ -1885,9 +1886,9 @@ xstring         : tXSTRING_BEG xstring_contents tSTRING_END {
                     ISourcePosition position = support.getPosition($2);
 
                     if ($2 == null) {
-                        $$ = new XStrNode(position, null);
+                        $$ = new XStrNode(position, null, StringSupport.CR_7BIT);
                     } else if ($2 instanceof StrNode) {
-                        $$ = new XStrNode(position, (ByteList) $<StrNode>2.getValue().clone());
+                        $$ = new XStrNode(position, (ByteList) $<StrNode>2.getValue().clone(), $<StrNode>2.getCodeRange());
                     } else if ($2 instanceof DStrNode) {
                         $$ = new DXStrNode(position, $<DStrNode>2);