More FIXME addressing and removal. Last ones I left should be conside…

…red as FIXMEs like others but I am leaving some with bytelist_love to give a pedigree to when it was made.
jruby · Apr 24, 2018 · 3488484 · 3488484
1 parent 69dc167
commit 3488484
Show file tree

Hide file tree

Showing 8 changed files with 21 additions and 29 deletions.
diff --git a/core/src/main/java/org/jruby/RubyModule.java b/core/src/main/java/org/jruby/RubyModule.java
@@ -4406,7 +4406,7 @@ protected final String validateConstant(IRubyObject name) {
         return symbol.idString();
     }
 
-    // FIXME: bytelist_love: This should really be working with symbol segments (errorName is FQN).
+    // FIXME: This should really be working with symbol segments (errorName is FQN).
     protected final String validateConstant(String name, IRubyObject errorName) {
         if (IdUtil.isValidConstantName19(name)) return name;
 

diff --git a/core/src/main/java/org/jruby/RubySymbol.java b/core/src/main/java/org/jruby/RubySymbol.java
@@ -221,7 +221,7 @@ public final boolean eql(IRubyObject other) {
         return other == this;
     }
 
-    // FIXME: bytelist_love: Symbol should get this set so we do not need to calculate it.
+    // FIXME: Symbol (like MRI) should get flag set for types of identifiers it can represent so we don't recalc this all the time (and others)
     /**
      * Is the string this constant represents a valid constant identifier name.
      */
@@ -330,7 +330,6 @@ public static RubySymbol newIDSymbol(Ruby runtime, ByteList bytes) {
      */
     public static RubySymbol newConstantSymbol(Ruby runtime, IRubyObject fqn, ByteList bytes) {
         if (bytes.length() == 0) {
-            // FIXME: bytelist_love: change NameError to accept symbol as name.
             throw runtime.newNameError(str(runtime, "wrong constant name ", ids(runtime, fqn)), "");
         }
 

diff --git a/core/src/main/java/org/jruby/ir/instructions/CallBase.java b/core/src/main/java/org/jruby/ir/instructions/CallBase.java
@@ -245,7 +245,7 @@ public boolean computeScopeFlags(IRScope scope) {
                 // either framed or scoped. Previously it only did this logic for
                 // send(:local_variables).
 
-                // FIXME: bytelist_love - This probably should be id-based and not get potentially encoded string.
+                // This says getString but I believe this will also always be an id string in this case so it is ok.
                 String sendName = ((StringLiteral) meth).getString();
                 if (MethodIndex.SCOPE_AWARE_METHODS.contains(sendName)) {
                     modifiedScope = true;

diff --git a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
@@ -1023,8 +1023,6 @@ public static IRubyObject getPreArgSafe(ThreadContext context, IRubyObject[] arg
         return result;
     }
 
-    // FIXME: bytelist_love - JIT still uses this and we use id string to retrieve symbol but JIT can maybe reify a symbol
-    // and then we can eliminate this method.
     public static IRubyObject receiveKeywordArg(ThreadContext context, IRubyObject[] args, int required, String id, boolean acceptsKeywordArgument) {
         RubyHash keywordArguments = extractKwargsHash(args, required, acceptsKeywordArgument);
 

diff --git a/core/src/main/java/org/jruby/lexer/yacc/RubyLexer.java b/core/src/main/java/org/jruby/lexer/yacc/RubyLexer.java
@@ -273,9 +273,10 @@ public static Keyword getKeyword(String str) {
     public int tokenize_ident(int result) {
         // FIXME: Get token from newtok index to lex_p?
         ByteList value = createTokenByteList();
+        Ruby runtime = parserSupport.getConfiguration().getRuntime();
+        String id = runtime.newSymbol(value).idString();
 
-        // FIXME: bytelist_love: use ID helper here once it is made.
-        if (isLexState(last_state, EXPR_DOT|EXPR_FNAME) && parserSupport.getCurrentScope().isDefined(value.toString()) >= 0) {
+        if (isLexState(last_state, EXPR_DOT|EXPR_FNAME) && parserSupport.getCurrentScope().isDefined(id) >= 0) {
             setState(EXPR_END);
         }
 
@@ -1071,9 +1072,11 @@ private int yylex() throws IOException {
     }
 
     private int identifierToken(int result, ByteList value) {
-        // FIXME: bytelist_love: use ID helper here once it is made.
+        Ruby runtime = parserSupport.getConfiguration().getRuntime();
+        String id = runtime.newSymbol(value).idString();
+
         if (result == RubyParser.tIDENTIFIER && !isLexState(last_state, EXPR_DOT|EXPR_FNAME) &&
-                parserSupport.getCurrentScope().isDefined(value.toString()) >= 0) {
+                parserSupport.getCurrentScope().isDefined(id) >= 0) {
             setState(EXPR_END|EXPR_LABEL);
         }
 

diff --git a/core/src/main/java/org/jruby/parser/ParserSupport.java b/core/src/main/java/org/jruby/parser/ParserSupport.java
@@ -255,9 +255,10 @@ public Node appendToBlock(Node head, Node tail) {
     }
 
     // We know it has to be tLABEL or tIDENTIFIER so none of the other assignable logic is needed
-    public AssignableNode assignableInCurr(ByteList name, Node value) {
-        currentScope.addVariableThisScope(ID(name));
-        return currentScope.assign(lexer.getPosition(), symbolID(name), makeNullNil(value));
+    public AssignableNode assignableInCurr(ByteList nameBytes, Node value) {
+        RubySymbol name = symbolID(nameBytes);
+        currentScope.addVariableThisScope(name.idString());
+        return currentScope.assign(lexer.getPosition(), name, makeNullNil(value));
     }
 
     public Node getOperatorCallNode(Node firstNode, ByteList operator) {
@@ -828,12 +829,6 @@ public RubySymbol symbolID(ByteList identifierValue) {
         return RubySymbol.newIDSymbol(getConfiguration().getRuntime(), identifierValue);
     }
 
-    // FIXME: bytelist_love: Move to common helper location
-    // Return raw 8859_1 string which is used as key for identifier in symbol table.
-    private static String ID(ByteList identifier) {
-        return identifier.toString();
-    }
-
     public Node newOpAsgn(ISourcePosition position, Node receiverNode, ByteList callType, Node valueNode, ByteList variableName, ByteList operatorName) {
         return new OpAsgnNode(position, receiverNode, valueNode, symbolID(variableName), symbolID(operatorName), isLazy(callType));
     }
@@ -1294,20 +1289,22 @@ public String formal_argument(String identifier) {
     }
 
     // 1.9
-    public ByteList shadowing_lvar(ByteList name) {
-        if (name.realSize() == 1 && name.charAt(0) == '_') return name;
+    public ByteList shadowing_lvar(ByteList nameBytes) {
+        if (nameBytes.realSize() == 1 && nameBytes.charAt(0) == '_') return nameBytes;
 
-        String id = ID(name);
+        RubySymbol name = symbolID(nameBytes);
+        String id = name.idString();
 
         StaticScope current = getCurrentScope();
         if (current.exists(id) >= 0) yyerror("duplicated argument name");
 
         if (current.isBlockScope() && warnings.isVerbose() && current.isDefined(id) >= 0 &&
                 Options.PARSER_WARN_LOCAL_SHADOWING.load()) {
-            warnings.warning(ID.STATEMENT_NOT_REACHED, lexer.getFile(), lexer.getPosition().getLine(), "shadowing outer local variable - " + StringSupport.byteListAsString(name));
+            Ruby runtime = getConfiguration().getRuntime();
+            warnings.warning(ID.STATEMENT_NOT_REACHED, lexer.getFile(), lexer.getPosition().getLine(), str(runtime, "shadowing outer local variable - ", ids(runtime, name)));
         }
 
-        return name;
+        return nameBytes;
     }
 
     @Deprecated

diff --git a/core/src/main/java/org/jruby/util/ByteListHelper.java b/core/src/main/java/org/jruby/util/ByteListHelper.java
@@ -1,10 +1,7 @@
 package org.jruby.util;
 
 import org.jcodings.Encoding;
-import org.jruby.Ruby;
-import org.jruby.RubyArray;
 
-// FIXME: bytelist_love: Should we expand ByteList as a library to include more of this stuff?
 /**
  * Helpers for working with bytelists.
  */

diff --git a/core/src/main/java/org/jruby/util/RubyStringBuilder.java b/core/src/main/java/org/jruby/util/RubyStringBuilder.java
@@ -48,10 +48,8 @@ public static RubyString ids(Ruby runtime, String id) {
      * @param object
      */
     public static RubyString ids(Ruby runtime, IRubyObject object) {
-        // FIXME: bytelist_love - should this be convertToString()?
         ByteList identifier;
 
-        // FIXME: bytelist_love - Want something which gets bytes without any dyncalls I think? [needs test]
         if (object instanceof RubyString) {
             identifier = ((RubyString) object).getByteList();
         } else if (object instanceof RubySymbol) {