add review comments

vyperlang · Oct 26, 2023 · fded614 · fded614
1 parent a5df008
commit fded614
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 1 deletion.
diff --git a/vyper/codegen/dfg.py b/vyper/codegen/dfg.py
@@ -259,15 +259,18 @@ def _generate_evm_for_basicblock_r(
 label_counter = 0
 
 
+# REVIEW: would this be better as a class?
 def _generate_evm_for_instruction_r(
     ctx: IRFunction, assembly: list, inst: IRInstruction, stack_map: StackMap
 ) -> list[str]:
     global label_counter
 
     for op in inst.get_output_operands():
         for target in ctx.dfg_inputs.get(op.value, []):
+            # REVIEW: what does this line do?
             if target.parent != inst.parent:
                 continue
+            # REVIEW: what does this line do?
             if target.fen != inst.fen:
                 continue
             assembly = _generate_evm_for_instruction_r(ctx, assembly, target, stack_map)
@@ -298,6 +301,8 @@ def _generate_evm_for_instruction_r(
     if opcode == "select":  # REVIEW: maybe call this 'phi'
         ret = inst.get_output_operands()[0]
         inputs = inst.get_input_operands()
+        # REVIEW: the special handling in get_depth_in for lists
+        # seems cursed, refactor
         depth = stack_map.get_depth_in(inputs)
         assert depth is not StackMap.NOT_IN_STACK, "Operand not in stack"
         to_be_replaced = stack_map.peek(depth)

diff --git a/vyper/compiler/utils.py b/vyper/compiler/utils.py
@@ -49,9 +49,12 @@ def _expand_row(row):
     return result
 
 
+# REVIEW: move this to vyper/ir/ or vyper/venom/
+# rename to StackModel
 class StackMap:
     NOT_IN_STACK = object()
-    stack_map: list[IRValueBase]
+    stack_map: list[IRValueBase]  # REVIEW: rename to stack
+    # REVIEW: dead variable
     dependant_liveness: OrderedSet[IRValueBase]
 
     def __init__(self):
@@ -93,12 +96,15 @@ def get_depth_in(self, op: IRValueBase | list[IRValueBase]) -> int:
 
         for i, stack_op in enumerate(self.stack_map[::-1]):
             if isinstance(stack_op, IRValueBase):
+                # REVIEW: handling literals this way seems a bit cursed,
+                # why not use IRLiteral, so it is always IRValueBase?
                 if isinstance(op, str) and stack_op.value == op:
                     return -i
                 if isinstance(op, int) and stack_op.value == op:
                     return -i
                 if isinstance(op, IRValueBase) and stack_op.value == op.value:
                     return -i
+                # REVIEW: this branch seems cursed
                 elif isinstance(op, list) and stack_op in op:
                     return -i