Skip to content

Commit

Permalink
Cleanup! Removed a bunch of stale FIXMEs.
Browse files Browse the repository at this point in the history 
* Cleaned up some comments.
* Removed some and tweaked some FIXMEs.
  • Loading branch information
@subbuss
subbuss committed Nov 1, 2014
1 parent f2c36e6 commit 8434fe5
Show file tree
Hide file tree
Showing 30 changed files with 55 additions and 147 deletions.
1 change: 0 additions & 1 deletion core/src/main/java/org/jruby/ir/CodeVersion.java
View file
Expand Up @@ -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() {
Expand Down
41 changes: 19 additions & 22 deletions core/src/main/java/org/jruby/ir/IRBuilder.java
View file
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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());
Expand All @@ -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);
Expand All @@ -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;
}

Expand Down Expand Up @@ -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.
*
Expand Down Expand Up @@ -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")) {
Expand Down Expand Up @@ -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()));
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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:
Expand All @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/jruby/ir/IRClosure.java
View file
Expand Up @@ -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;
Expand Down
2 changes: 0 additions & 2 deletions core/src/main/java/org/jruby/ir/IRManager.java
View file
Expand Up @@ -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";
Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/org/jruby/ir/IRMethod.java
View file
Expand Up @@ -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;

Expand Down
23 changes: 10 additions & 13 deletions core/src/main/java/org/jruby/ir/IRScope.java
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down
3 changes: 1 addition & 2 deletions core/src/main/java/org/jruby/ir/Operation.java
View file
Expand Up @@ -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 */
Expand Down Expand Up @@ -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),
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/jruby/ir/dataflow/FlowGraphNode.java
View file
Expand Up @@ -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);
}

Expand Down
1 change: 0 additions & 1 deletion core/src/main/java/org/jruby/ir/dataflow/analyses/LiveVariablesProblem.java
View file
Expand Up @@ -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;
Expand Down
10 changes: 0 additions & 10 deletions core/src/main/java/org/jruby/ir/instructions/BuildCompoundArrayInstr.java
View file
Expand Up @@ -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;
}
Expand Down
17 changes: 0 additions & 17 deletions core/src/main/java/org/jruby/ir/instructions/BuildCompoundStringInstr.java
View file
Expand Up @@ -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;
}
Expand All @@ -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);
Expand Down
14 changes: 0 additions & 14 deletions core/src/main/java/org/jruby/ir/instructions/CallBase.java
View file
Expand Up @@ -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() {
Expand Down
1 change: 0 additions & 1 deletion core/src/main/java/org/jruby/ir/instructions/DefineClassInstr.java
View file
Expand Up @@ -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));
}

Expand Down
3 changes: 0 additions & 3 deletions core/src/main/java/org/jruby/ir/instructions/DefineClassMethodInstr.java
View file
@@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
7 changes: 0 additions & 7 deletions core/src/main/java/org/jruby/ir/instructions/Instr.java
View file
Expand Up @@ -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;
}
Expand Down
2 changes: 0 additions & 2 deletions core/src/main/java/org/jruby/ir/instructions/LexicalSearchConstInstr.java
View file
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/jruby/ir/instructions/PutClassVariableInstr.java
View file
Expand Up @@ -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;
Expand Down

0 comments on commit 8434fe5

Please sign in to comment.