Cleanup! Removed a bunch of stale FIXMEs.

* Cleaned up some comments. * Removed some and tweaked some FIXMEs.
jruby · Nov 1, 2014 · 8434fe5 · 8434fe5
1 parent f2c36e6
commit 8434fe5
Showing 30 changed files with 55 additions and 147 deletions.
diff --git a/core/src/main/java/org/jruby/ir/CodeVersion.java b/core/src/main/java/org/jruby/ir/CodeVersion.java
@@ -3,7 +3,6 @@
 public class CodeVersion {
     private static long _nextVersionNumber = 0L;
 
-    // SSS FIXME: Does int suffice, or do we need long?
     public final long _version;
 
     public static CodeVersion getClassVersionToken()  {

diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -526,8 +526,7 @@ public Operand buildLambda(LambdaNode node, IRScope s) {
         handleBreakAndReturnsInLambdas(closure);
 
         Variable lambda = s.createTemporaryVariable();
-        // SSS FIXME: Is this the right self here?
-        WrappedIRClosure lambdaBody = new WrappedIRClosure(s.getSelf(), closure);
+        WrappedIRClosure lambdaBody = new WrappedIRClosure(closure.getSelf(), closure);
         addInstr(s, new BuildLambdaInstr(lambda, lambdaBody, node.getPosition()));
         return lambda;
     }
@@ -1024,7 +1023,7 @@ public Operand buildCase(CaseNode caseNode, IRScope s) {
             labels.add(bodyLabel);
             Operand v1, v2;
             if (whenNode.getExpressionNodes() instanceof ListNode) {
-                // SSS FIXME: Note about refactoring:
+                // Note about refactoring:
                 // - BEQInstr has a quick implementation when the second operand is a boolean literal
                 //   If it can be fixed to do this even on the first operand, we can switch around
                 //   v1 and v2 in the UndefinedValue scenario and DRY out this code.
@@ -1053,7 +1052,7 @@ public Operand buildCase(CaseNode caseNode, IRScope s) {
 
             // SSS FIXME: This doesn't preserve original order of when clauses.  We could consider
             // preserving the order (or maybe not, since we would have to sort the constants first
-            // in any case) for outputing jump tables in certain situations.
+            // in any case) for outputting jump tables in certain situations.
             //
             // add body to map for emitting later
             bodies.put(bodyLabel, whenNode.getBodyNode());
@@ -1062,13 +1061,13 @@ public Operand buildCase(CaseNode caseNode, IRScope s) {
         // Jump to else in case nothing matches!
         addInstr(s, new JumpInstr(elseLabel));
 
-        // build "else" if it exists
+        // Build "else" if it exists
         if (hasElse) {
             labels.add(elseLabel);
             bodies.put(elseLabel, caseNode.getElseNode());
         }
 
-        // now emit bodies while preserving when clauses order
+        // Now, emit bodies while preserving when clauses order
         for (Label whenLabel: labels) {
             addInstr(s, new LabelInstr(whenLabel));
             Operand bodyValue = build(bodies.get(whenLabel), s);
@@ -1089,11 +1088,9 @@ public Operand buildCase(CaseNode caseNode, IRScope s) {
             addInstr(s, new JumpInstr(endLabel));
         }
 
-        // close it out
+        // Close it out
         addInstr(s, new LabelInstr(endLabel));
 
-        // SSS: Got rid of the marker case label instruction
-
         return result;
     }
 
@@ -1239,7 +1236,7 @@ private Operand searchConst(IRScope s, String name) {
         boolean noPrivateConstants = false;
         Variable v = s.createTemporaryVariable();
 /**
- * SSS FIXME: Go back to a single instruction for now.
+ * SSS FIXME: Went back to a single instruction for now.
  *
  * Do not split search into lexical-search, inheritance-search, and const-missing instrs.
  *
@@ -1745,7 +1742,6 @@ public void receiveRequiredArg(Node node, IRScope s, int argIndex, boolean post,
                 ArgumentNode a = (ArgumentNode)node;
                 String argName = a.getName();
                 if (s instanceof IRMethod) ((IRMethod)s).addArgDesc(IRMethodArgs.ArgType.req, argName);
-                // SSS FIXME: _$0 feels fragile?
                 // Ignore duplicate "_" args in blocks
                 // (duplicate _ args are named "_$0")
                 if (!argName.equals("_$0")) {
@@ -1796,10 +1792,11 @@ protected void receiveNonBlockArgs(final ArgsNode argsNode, IRScope s) {
 
         // For closures, we don't need the check arity call
         if (s instanceof IRMethod) {
-            // FIXME: Expensive to do this explicitly?  But, two advantages:
+            // Expensive to do this explicitly?  But, two advantages:
             // (a) on inlining, we'll be able to get rid of these checks in almost every case.
             // (b) compiler to bytecode will anyway generate this and this is explicit.
-            // For now, we are going explicit instruction route.  But later, perhaps can make this implicit in the method setup preamble?
+            // For now, we are going explicit instruction route.
+            // But later, perhaps can make this implicit in the method setup preamble?
 
             addInstr(s, new CheckArityInstr(required, opt, rest, argsNode.hasKwargs(),
                     keyRest == null ? -1 : keyRest.getIndex()));
@@ -2817,13 +2814,13 @@ public Operand buildOpAsgnAnd(OpAsgnAndNode andNode, IRScope s) {
     // to make sure the value is not defined but nil.  Nil will trigger ||=
     // rhs expression.
     //
-    // Translate "x ||= y" --> "x = (is_defined(x) && is_true(x) ? x : y)" -->
-    //
-    //    v = -- build(x) should return a variable! --
-    //    f = is_true(v)
-    //    beq(f, true, L)
-    //    -- build(x = y) --
-    // L:
+    // "x ||= y"
+    // --> "x = (is_defined(x) && is_true(x) ? x : y)"
+    // --> v = -- build(x) should return a variable! --
+    //     f = is_true(v)
+    //     beq(f, true, L)
+    //     -- build(x = y) --
+    //   L:
     //
     public Operand buildOpAsgnOr(final OpAsgnOrNode orNode, IRScope s) {
         Label    l1 = s.getNewLabel();
@@ -3141,7 +3138,7 @@ private void buildRescueBodyInternal(IRScope s, RescueBodyNode rescueBodyNode, V
                 outputExceptionCheck(s, build(exceptionList, s), exc, caughtLabel);
             }
         } else {
-            // FIXME:
+            // SSS FIXME:
             // rescue => e AND rescue implicitly EQQ the exception object with StandardError
             // We generate explicit IR for this test here.  But, this can lead to inconsistent
             // behavior (when compared to MRI) in certain scenarios.  See example:
@@ -3152,7 +3149,7 @@ private void buildRescueBodyInternal(IRScope s, RescueBodyNode rescueBodyNode, V
             // MRI rescues the error, but we will raise an exception because of reassignment
             // of StandardError.  I am ignoring this for now and treating this as undefined behavior.
             //
-            // SSS FIXME: Create a 'StandardError' operand type to eliminate this.
+            // Solution: Create a 'StandardError' operand type to eliminate this.
             Variable v = addResultInstr(s, new InheritanceSearchConstInstr(s.createTemporaryVariable(), s.getCurrentModuleVariable(), "StandardError", false));
             outputExceptionCheck(s, v, exc, caughtLabel);
         }

diff --git a/core/src/main/java/org/jruby/ir/IRClosure.java b/core/src/main/java/org/jruby/ir/IRClosure.java
@@ -293,7 +293,7 @@ public int getNestingDepth() {
 
     protected IRClosure cloneForInlining(CloneInfo ii, IRClosure clone) {
         clone.nestingDepth  = this.nestingDepth;
-        // FIXME: This is fragile. Untangle this state.
+        // SSS FIXME: This is fragile. Untangle this state.
         // Why is this being copied over to InterpretedIRBlockBody?
         clone.setParameterList(this.parameterList);
         clone.isBeginEndBlock = this.isBeginEndBlock;

diff --git a/core/src/main/java/org/jruby/ir/IRManager.java b/core/src/main/java/org/jruby/ir/IRManager.java
@@ -14,8 +14,6 @@
 import java.util.List;
 import java.util.Set;
 
-/**
- */
 public class IRManager {
     public static final String SAFE_COMPILER_PASSES = "";
     public static final String DEFAULT_COMPILER_PASSES = "OptimizeTempVarsPass,LocalOptimizationPass";

diff --git a/core/src/main/java/org/jruby/ir/IRMethod.java b/core/src/main/java/org/jruby/ir/IRMethod.java
@@ -19,8 +19,9 @@
 public class IRMethod extends IRScope {
     public final boolean isInstanceMethod;
 
-    // SSS FIXME: Note that if operands from the method are modified,
+    // Note that if operands from the method are modified,
     // callArgs would have to be updated as well
+    //
     // Call parameters
     private List<Operand> callArgs;
 

diff --git a/core/src/main/java/org/jruby/ir/IRScope.java b/core/src/main/java/org/jruby/ir/IRScope.java
@@ -514,7 +514,7 @@ protected Instr[] prepareInstructions() {
             BasicBlock rescuerBB = cfg().getRescuerBBFor(b);
             int rescuerPC = rescuerBB == null ? -1 : rescuerBB.getLabel().getTargetPC();
             for (Instr instr : b.getInstrs()) {
-                // FIXME: If we did not omit instrs from previous pass we could end up just doing a
+                // FIXME: If we did not omit instrs from previous pass, we could end up just doing
                 // a size and for loop this n times instead of walking an examining each instr
                 if (!(instr instanceof ReceiveSelfInstr)) {
                     linearizedInstrArray[ipc].setRPC(rescuerPC);
@@ -542,19 +542,18 @@ public List<CompilerPass> getExecutedPasses() {
         return executedPasses;
     }
 
+    // SSS FIXME: We should configure different optimization levels
+    // and run different kinds of analysis depending on time budget.
+    // Accordingly, we need to set IR levels/states (basic, optimized, etc.)
+    // ENEBO: If we use a MT optimization mechanism we cannot mutate CFG
+    // while another thread is using it.  This may need to happen on a clone()
+    // and we may need to update the method to return the new method.  Also,
+    // if this scope is held in multiple locations how do we update all references?
     private void runCompilerPasses(List<CompilerPass> passes) {
         // SSS FIXME: Why is this again?  Document this weirdness!
         // Forcibly clear out the shared eval-scope variable allocator each time this method executes
         initEvalScopeVariableAllocator(true);
 
-        // SSS FIXME: We should configure different optimization levels
-        // and run different kinds of analysis depending on time budget.
-        // Accordingly, we need to set IR levels/states (basic, optimized, etc.)
-        // ENEBO: If we use a MT optimization mechanism we cannot mutate CFG
-        // while another thread is using it.  This may need to happen on a clone()
-        // and we may need to update the method to return the new method.  Also,
-        // if this scope is held in multiple locations how do we update all references?
-
         // All passes are disabled in scopes where BEGIN and END scopes might
         // screw around with escaped variables. Optimizing for them is not
         // worth the effort. It is simpler to just go fully safe in scopes
@@ -628,7 +627,6 @@ public synchronized InterpreterContext prepareForInterpretation() {
         return interpreterContext;
     }
 
-    /* SSS FIXME: Do we need to synchronize on this?  Cache this info in a scope field? */
     /** Run any necessary passes to get the IR ready for compilation */
     public synchronized List<BasicBlock> prepareForCompilation() {
         // Reset linearization, if any exists
@@ -827,8 +825,7 @@ public Map<String, LocalVariable> getLocalVariables() {
     }
 
     /**
-     * Set the local variables for this scope. This should only be used by persistence
-     * layer.
+     * Set the local variables for this scope. This should only be used by persistence layer.
      */
     // FIXME: Consider making constructor for persistence to pass in all of this stuff
     public void setLocalVariables(Map<String, LocalVariable> variables) {
@@ -1179,7 +1176,7 @@ public boolean isBeginEndBlock() {
     }
 
     /**
-     * Does this scope represent a module body?  (SSS FIXME: what about script or eval script bodies?)
+     * Does this scope represent a module body?
      */
     public boolean isModuleBody() {
         return false;

diff --git a/core/src/main/java/org/jruby/ir/Operation.java b/core/src/main/java/org/jruby/ir/Operation.java
@@ -116,7 +116,6 @@ public enum Operation {
     CONST_MISSING(OpFlags.f_can_raise_exception),
     SEARCH_CONST(OpFlags.f_can_raise_exception),
 
-    /** value loads (SSS FIXME: Do any of these have side effects?) **/
     GET_GLOBAL_VAR(OpFlags.f_is_load),
     GET_FIELD(OpFlags.f_is_load),
     /** SSS FIXME: Document what causes this instr to raise an exception */
@@ -170,7 +169,7 @@ public enum Operation {
     DEFINED_CONSTANT_OR_METHOD(OpFlags.f_can_raise_exception),
     GET_ERROR_INFO(0),
     METHOD_DEFINED(OpFlags.f_can_raise_exception),
-    RESTORE_ERROR_INFO(OpFlags.f_has_side_effect), // SSS FIXME: Side effecting? Really?
+    RESTORE_ERROR_INFO(OpFlags.f_has_side_effect),
 
     /* Boxing/Unboxing between Ruby <--> Java types */
     BOX_FIXNUM(0),

diff --git a/core/src/main/java/org/jruby/ir/dataflow/FlowGraphNode.java b/core/src/main/java/org/jruby/ir/dataflow/FlowGraphNode.java
@@ -21,7 +21,7 @@ public FlowGraphNode(T problem, BasicBlock basicBlock) {
         this.problem = problem;
         this.basicBlock = basicBlock;
 
-        // FIXME: Enebo: Does this really needed to be calculaed here?  Can analysis change this value?
+        // Cache the rescuer node for easy access
         rescuer = problem.getScope().cfg().getRescuerBBFor(basicBlock);
     }
 

diff --git a/core/src/main/java/org/jruby/ir/dataflow/analyses/LiveVariablesProblem.java b/core/src/main/java/org/jruby/ir/dataflow/analyses/LiveVariablesProblem.java
@@ -165,7 +165,6 @@ public String getName() {
     private HashMap<Variable, Integer> dfVarMap = new HashMap<Variable, Integer>();
     private HashMap<Integer, Variable> varDfVarMap = new HashMap<Integer, Variable>();
     private HashSet<LocalVariable> localVars = new HashSet<LocalVariable>(); // Local variables that can be live across dataflow barriers
-    // SSS FIXME: Should this be part of IRScope??
 
     // Variables that cross eval boundaries and are always live in this scope
     private List<LocalVariable> alwaysLiveVars;

diff --git a/core/src/main/java/org/jruby/ir/instructions/BuildCompoundArrayInstr.java b/core/src/main/java/org/jruby/ir/instructions/BuildCompoundArrayInstr.java
@@ -42,20 +42,10 @@ public void updateResult(Variable v) {
         this.result = v;
     }
 
-    public Operand getA1() {
-        return a1;
-    }
-
-    public Operand getA2() {
-        return a2;
-    }
-
-    // SSS FIXME: Consolidate identical methods
     public Operand getAppendingArg() {
         return a1;
     }
 
-    // SSS FIXME: Consolidate identical methods
     public Operand getAppendedArg() {
         return a2;
     }

diff --git a/core/src/main/java/org/jruby/ir/instructions/BuildCompoundStringInstr.java b/core/src/main/java/org/jruby/ir/instructions/BuildCompoundStringInstr.java
@@ -77,17 +77,6 @@ public Instr clone(CloneInfo ii) {
         return new BuildCompoundStringInstr(ii.getRenamedVariable(result), newPieces, encoding);
     }
 
-    // SSS FIXME: Buggy?
-    String retrieveJavaString(ThreadContext context, IRubyObject self, StaticScope currScope, DynamicScope currDynScope, Object[] temp) {
-        StringBuilder buf = new StringBuilder();
-
-        for (Operand p : pieces) {
-            buf.append(p.retrieve(context, self, currScope, currDynScope, temp));
-        }
-
-        return buf.toString();
-    }
-
     public boolean isSameEncoding(StringLiteral str) {
         return str.bytelist.getEncoding() == encoding;
     }
@@ -103,12 +92,6 @@ public RubyString[] retrievePieces(ThreadContext context, IRubyObject self, Stat
 
     @Override
     public Object interpret(ThreadContext context, StaticScope currScope, DynamicScope currDynScope, IRubyObject self, Object[] temp) {
-        // SSS FIXME: Doesn't work in all cases.  See example below
-        //
-        //    s = "x\234\355\301\001\001\000\000\000\200\220\376\257\356\b\n#{"\000" * 31}\030\200\000\000\001"
-        //    s.length prints 70 instead of 52
-        //
-        // return context.getRuntime().newString(retrieveJavaString(interp, context, self));
 
         ByteList bytes = new ByteList();
         bytes.setEncoding(encoding);

diff --git a/core/src/main/java/org/jruby/ir/instructions/CallBase.java b/core/src/main/java/org/jruby/ir/instructions/CallBase.java
@@ -244,20 +244,6 @@ public boolean isStaticCallTarget() {
         return false;
     }
 
-    // SSS FIXME: Unused currently.
-    // Can this call lead to ruby code getting modified?
-    // If we don't know what method we are calling, we assume it can (pessimistic, but safe!)
-    public boolean canModifyCode() {
-        return true;
-    }
-
-    // SSS FIXME: Unused currently.
-    // Regexp and IO calls can do this -- and since we do not know at IR-build time
-    // what the call target is, we have to conservatively assume yes
-    public boolean canSetDollarVars() {
-        return true;
-    }
-
     // SSS FIXME: Are all bases covered?
     // How about aliasing of 'call', 'eval', 'send', 'module_eval', 'class_eval', 'instance_eval'?
     private boolean computeEvalFlag() {

diff --git a/core/src/main/java/org/jruby/ir/instructions/DefineClassInstr.java b/core/src/main/java/org/jruby/ir/instructions/DefineClassInstr.java
@@ -60,7 +60,6 @@ public String toString() {
 
     @Override
     public Instr clone(CloneInfo ii) {
-        // SSS FIXME: So, do we clone the class body scope or not?
         return new DefineClassInstr(ii.getRenamedVariable(result), this.newIRClassBody, container.cloneForInlining(ii), superClass.cloneForInlining(ii));
     }
 

diff --git a/core/src/main/java/org/jruby/ir/instructions/DefineClassMethodInstr.java b/core/src/main/java/org/jruby/ir/instructions/DefineClassMethodInstr.java
@@ -1,6 +1,5 @@
 package org.jruby.ir.instructions;
 
-import org.jruby.Ruby;
 import org.jruby.ir.IRMethod;
 import org.jruby.ir.IRVisitor;
 import org.jruby.ir.Operation;
@@ -15,7 +14,6 @@
 import java.util.Map;
 
 // SSS FIXME: Should we merge DefineInstanceMethod and DefineClassMethod instructions?
-// identical except for 1 bit in interpret -- or will they diverge?
 public class DefineClassMethodInstr extends Instr implements FixedArityInstr {
     private Operand container;
     private final IRMethod method;
@@ -54,7 +52,6 @@ public Instr clone(CloneInfo ii) {
         return new DefineClassMethodInstr(container.cloneForInlining(ii), method);
     }
 
-    // SSS FIXME: Go through this and DefineInstanceMethodInstr.interpret, clean up, extract common code
     @Override
     public Object interpret(ThreadContext context, StaticScope currScope, DynamicScope currDynScope, IRubyObject self, Object[] temp) {
         IRubyObject obj = (IRubyObject) container.retrieve(context, self, currScope, currDynScope, temp);

diff --git a/core/src/main/java/org/jruby/ir/instructions/Instr.java b/core/src/main/java/org/jruby/ir/instructions/Instr.java
@@ -110,13 +110,6 @@ public boolean canBeDeleted(IRScope s) {
                  // would use %block.  In that scenario, we cannot delete the '%block = recv_closure' instruction.
                  // This is safe, but conservative.
                  return false;
-             } else if (s.usesZSuper() && getOperation().isArgReceive()) {
-                 // If this scope (or any nested scope) has a ZSuperInstr, then the arguments of this
-                 // scope could be used by any of those ZSuper instructions.  If so, we cannot delete
-                 // the argument receive.
-                 // SSS FIXME: This check may be redundant. ZSuper now explicits lists
-                 // all arguments that it may use.
-                 return false;
              } else {
                  return true;
              }

diff --git a/core/src/main/java/org/jruby/ir/instructions/LexicalSearchConstInstr.java b/core/src/main/java/org/jruby/ir/instructions/LexicalSearchConstInstr.java
@@ -80,8 +80,6 @@ public Instr clone(CloneInfo ii) {
     private Object cache(ThreadContext context, StaticScope currScope, DynamicScope currDynScope, IRubyObject self, Object[] temp) {
         StaticScope staticScope = (StaticScope) definingScope.retrieve(context, self, currScope, currDynScope, temp);
 
-        // CON FIXME: Removed SSS hack for IRManager objects not having a static scope, so we can find and fix
-
         IRubyObject constant = staticScope.getConstantInner(constName);
 
         if (constant == null) {

diff --git a/core/src/main/java/org/jruby/ir/instructions/PutClassVariableInstr.java b/core/src/main/java/org/jruby/ir/instructions/PutClassVariableInstr.java
@@ -28,7 +28,7 @@ public Object interpret(ThreadContext context, StaticScope currScope, DynamicSco
 
         assert module != null : "MODULE should always be something";
 
-		  // SSS FIXME: What is this check again???
+		// SSS FIXME: What is this check again???
         // Modules and classes set this constant as a side-effect
         if (!(getValue() instanceof CurrentScope)) module.setClassVar(getRef(), value);
         return null;

diff --git a/core/src/main/java/org/jruby/ir/instructions/RaiseRequiredKeywordArgumentError.java b/core/src/main/java/org/jruby/ir/instructions/RaiseRequiredKeywordArgumentError.java
@@ -9,8 +9,6 @@
 import org.jruby.runtime.ThreadContext;
 import org.jruby.runtime.builtin.IRubyObject;
 
-/**
- */
 // FIXME: Consider making argument error a single more generic instruction and combining with RaiseArgumentError
 public class RaiseRequiredKeywordArgumentError extends Instr implements FixedArityInstr {
     private String name;

diff --git a/core/src/main/java/org/jruby/ir/instructions/ReceiveClosureInstr.java b/core/src/main/java/org/jruby/ir/instructions/ReceiveClosureInstr.java
@@ -48,6 +48,8 @@ public boolean computeScopeFlags(IRScope scope) {
     public Instr clone(CloneInfo info) {
         if (info instanceof SimpleCloneInfo) return new ReceiveClosureInstr(info.getRenamedVariable(result));
 
+        // SSS FIXME: This code below is for inlining and is untested.
+
         InlineCloneInfo ii = (InlineCloneInfo) info;
 
         // SSS FIXME: This is not strictly correct -- we have to wrap the block into an

diff --git a/core/src/main/java/org/jruby/ir/instructions/ThrowExceptionInstr.java b/core/src/main/java/org/jruby/ir/instructions/ThrowExceptionInstr.java
@@ -60,7 +60,6 @@ public Object interpret(ThreadContext context, StaticScope currScope, DynamicSco
         if (excObj instanceof IRubyObject) {
             RubyKernel.raise(context, context.runtime.getKernel(), new IRubyObject[] {(IRubyObject)excObj}, Block.NULL_BLOCK);
         } else if (excObj instanceof Throwable) { // java exception -- avoid having to add 'throws' clause everywhere!
-            // SSS FIXME: Can avoid this workaround by adding a case for this instruction in the interpreter loop
             Helpers.throwException((Throwable)excObj);
         }
 

diff --git a/core/src/main/java/org/jruby/ir/interpreter/Interpreter.java b/core/src/main/java/org/jruby/ir/interpreter/Interpreter.java
@@ -102,10 +102,10 @@ public static IRubyObject interpretCommonEval(Ruby runtime, String file, int lin
             evalScript.getStaticScope().setIRScope(evalScript);
             s.growIfNeeded();
 
-            runBeginEndBlocks(evalScript.getBeginBlocks(), context, self, ss, null); // FIXME: No temp vars yet right?
+            runBeginEndBlocks(evalScript.getBeginBlocks(), context, self, ss, null);
             rv = evalScript.call(context, self, evalScript.getStaticScope().getModule(), block, backtraceName);
         } finally {
-            runEndBlocks(ic.getEndBlocks(), context, self, ss, null); // FIXME: No temp vars right?
+            runEndBlocks(ic.getEndBlocks(), context, self, ss, null);
             s.clearEvalType();
             context.popScope();
         }
@@ -743,8 +743,6 @@ public static IRubyObject evalSimple(ThreadContext context, IRubyObject self, Ru
         try {
             Node node = runtime.parseEval(source.getByteList(), file, evalScope, lineNumber);
 
-            // SSS FIXME: AST interpreter passed both a runtime (which comes from the source string)
-            // and the thread-context rather than fetch one from the other.  Why is that?
             return Interpreter.interpretSimpleEval(runtime, file, lineNumber, "(eval)", node, self, evalType);
 /*
  * SSS FIXME: Why was this here?
@@ -789,8 +787,6 @@ public static IRubyObject evalWithBinding(ThreadContext context, IRubyObject sel
             Node node = runtime.parseEval(source.getByteList(), binding.getFile(), evalScope, binding.getLine());
             Block block = binding.getFrame().getBlock();
 
-            // SSS FIXME: AST interpreter passed both a runtime (which comes from the source string)
-            // and the thread-context rather than fetch one from the other.  Why is that?
             return Interpreter.interpretBindingEval(runtime, binding.getFile(), binding.getLine(), binding.getMethod(), node, self, block);
         } catch (StackOverflowError soe) {
             throw runtime.newSystemStackError("stack level too deep", soe);

diff --git a/core/src/main/java/org/jruby/ir/operands/ClosureLocalVariable.java b/core/src/main/java/org/jruby/ir/operands/ClosureLocalVariable.java
@@ -37,7 +37,6 @@ public Variable clone(SimpleCloneInfo ii) {
         return new ClosureLocalVariable((IRClosure) ii.getScope(), name, scopeDepth, offset);
     }
 
-    // SSS FIXME: Better name than this?
     public LocalVariable cloneForDepth(int n) {
         return new ClosureLocalVariable(definingScope, name, n, offset);
     }

diff --git a/core/src/main/java/org/jruby/ir/operands/LocalVariable.java b/core/src/main/java/org/jruby/ir/operands/LocalVariable.java
@@ -1,8 +1,3 @@
-/*
- * To change this template, choose Tools | Templates
- * and open the template in the editor.
- */
-
 package org.jruby.ir.operands;
 
 import org.jruby.ir.IRVisitor;
@@ -12,9 +7,6 @@
 import org.jruby.runtime.ThreadContext;
 import org.jruby.runtime.builtin.IRubyObject;
 
-/**
- * @author enebo
- */
 public class LocalVariable extends Variable implements DepthCloneable {
     protected final String name;
     protected final int scopeDepth;
@@ -93,7 +85,6 @@ public Variable clone(SimpleCloneInfo ii) {
         return new LocalVariable(name, scopeDepth, offset);
     }
 
-    // SSS FIXME: Better name than this?
     public LocalVariable cloneForDepth(int n) {
         return new LocalVariable(name, n, offset);
     }

diff --git a/core/src/main/java/org/jruby/ir/operands/Splat.java b/core/src/main/java/org/jruby/ir/operands/Splat.java
@@ -46,13 +46,6 @@ public Operand getArray() {
     @Override
     public Operand getSimplifiedOperand(Map<Operand, Operand> valueMap, boolean force) {
         Operand newArray = array.getSimplifiedOperand(valueMap, force);
-        /*
-         * SSS FIXME:  Cannot convert this to an Array operand!
-         *
-        if (_array instanceof Variable) {
-        _array = ((Variable)_array).getValue(valueMap);
-        }
-         */
         return (newArray == array) ? this : new Splat(newArray, unsplatArgs);
     }
 

diff --git a/core/src/main/java/org/jruby/ir/passes/AddCallProtocolInstructions.java b/core/src/main/java/org/jruby/ir/passes/AddCallProtocolInstructions.java
@@ -36,9 +36,6 @@ public Object execute(IRScope scope, Object... data) {
         // They dont push/pop a frame and do other special things like run begin/end blocks.
         // So, for now, they go through the runtime stub in IRScriptBody.
         //
-        // SSS FIXME: Right now, we always add push/pop frame instrs -- in the future, we may skip them
-        // for certain scopes.
-        //
         // Add explicit frame and binding push/pop instrs ONLY for methods -- we cannot handle this in closures and evals yet
         // If the scope uses $_ or $~ family of vars, has local load/stores, or if its binding has escaped, we have
         // to allocate a dynamic scope for it and add binding push/pop instructions.

diff --git a/core/src/main/java/org/jruby/ir/passes/DominatorTreeBuilder.java b/core/src/main/java/org/jruby/ir/passes/DominatorTreeBuilder.java
@@ -8,6 +8,7 @@
 
 import java.util.*;
 
+/* SSS: Currently unused code. Will be useful for some SSA building algos. */
 public class DominatorTreeBuilder extends CompilerPass {
     private static final Logger LOG = LoggerFactory.getLogger("DominatorTreeBuilder");
     public static List<Class<? extends CompilerPass>> DEPENDENCIES = Arrays.<Class<? extends CompilerPass>>asList(CFGBuilder.class);
@@ -37,7 +38,6 @@ public Object execute(IRScope scope, Object... data) {
 
     @Override
     public boolean invalidate(IRScope scope) {
-        // FIXME: We never store our dominator tree anywhere right?
         return false;
     }
 

diff --git a/core/src/main/java/org/jruby/ir/passes/LocalOptimizationPass.java b/core/src/main/java/org/jruby/ir/passes/LocalOptimizationPass.java
@@ -38,7 +38,8 @@ public Object execute(IRScope s, Object... data) {
         }
 
         // SSS FIXME: What is this about? 
-        // Why 'Only after running local opts'?  Figure out and document.
+        // Why 'Only after running local opts'? Figure out and document.
+        //
         // Only after running local opts, compute various execution scope flags.
         s.computeScopeFlags();
 
@@ -70,18 +71,6 @@ private static void recordSimplification(Variable res, Operand val, Map<Operand,
 
     public static void runLocalOptsOnInstrList(IRScope s, ListIterator<Instr> instrs, boolean preCFG) {
         // Reset value map if this instruction is the start/end of a basic block
-        //
-        // Right now, calls are considered hard boundaries for optimization and
-        // information cannot be propagated across them!
-        //
-        // SSS FIXME: Rather than treat all calls with a broad brush, what we need
-        // is to capture different attributes about a call :
-        //   - uses closures
-        //   - known call target
-        //   - can modify scope,
-        //   - etc.
-        //
-        // This information is probably already present in the AST Inspector
         Map<Operand,Operand> valueMap = new HashMap<Operand,Operand>();
         Map<Variable,List<Variable>> simplificationMap = new HashMap<Variable,List<Variable>>();
         while (instrs.hasNext()) {
@@ -133,6 +122,18 @@ public static void runLocalOptsOnInstrList(IRScope s, ListIterator<Instr> instrs
             }
 
             // If the call has been optimized away in the previous step, it is no longer a hard boundary for opts!
+            //
+            // Right now, calls are considered hard boundaries for optimization and
+            // information cannot be propagated across them!
+            //
+            // SSS FIXME: Rather than treat all calls with a broad brush, what we need
+            // is to capture different attributes about a call :
+            //   - uses closures
+            //   - known call target
+            //   - can modify scope,
+            //   - etc.
+            //
+            // This information is present in instruction flags on CallBase. Use it!
             if ((preCFG && iop.endsBasicBlock()) || (iop.isCall() && !i.isDead())) {
                 valueMap = new HashMap<Operand,Operand>();
                 simplificationMap = new HashMap<Variable,List<Variable>>();

diff --git a/core/src/main/java/org/jruby/ir/passes/UnboxingPass.java b/core/src/main/java/org/jruby/ir/passes/UnboxingPass.java
@@ -29,8 +29,6 @@ public Object execute(IRScope scope, Object... data) {
     }
 
     public boolean invalidate(IRScope scope) {
-        // FIXME: Can we reset this?
-        // Not right now.
         return false;
     }
 }
diff --git a/core/src/main/java/org/jruby/ir/representations/CFG.java b/core/src/main/java/org/jruby/ir/representations/CFG.java
@@ -505,8 +505,6 @@ public void removeBB(BasicBlock b) {
         graph.removeVertexFor(b);
         bbMap.remove(b.getLabel());
         rescuerMap.remove(b);
-
-        // SSS FIXME: Patch up rescued regions as well??
     }
 
     /**

diff --git a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
@@ -196,7 +196,7 @@ public static IRubyObject handlePropagatedBreak(ThreadContext context, DynamicSc
             if (isDebug()) System.out.println("---> Break reached target in scope: " + dynScope);
             return bj.breakValue;
 /* ---------------------------------------------------------------
- * FIXME: Puzzled .. Why is this not needed?
+ * SSS FIXME: Puzzled .. Why is this not needed?
         } else if (!context.scopeExistsOnCallStack(bj.scopeToReturnTo.getStaticScope())) {
             throw IRException.BREAK_LocalJumpError.getException(context.runtime);
  * --------------------------------------------------------------- */