rewrite BoolBinopNode for the non-boolean result case to avoid potentially...

rewrite BoolBinopNode for the non-boolean result case to avoid potentially harmful coercions along the way

CHANGES.rst

@@ -21,6 +21,13 @@ Features added
   is raised only when such a pointer is assigned to a variable and
   would thus exceed the lifetime of the string itself.
 
+* The "and"/"or" operators try to avoid unnecessary coercions of their
+  arguments.  They now evaluate the truth value of each argument
+  independently and only coerce the final result of the whole expression
+  to the target type (e.g. the type on the left side of an assignment).
+  This also avoids reference counting overhead for Python values during
+  evaluation and generally improves the code flow in the generated C code.
+
 * Cascaded assignments (a = b = ...) try to minimise the number of
   type coercions.
 

Cython/Compiler/ExprNodes.py

@@ -9628,6 +9628,9 @@ class BoolBinopNode(ExprNode):
     #  operand2     ExprNode
 
     subexprs = ['operand1', 'operand2']
+    operator = None
+    operand1 = None
+    operand2 = None
 
     def infer_type(self, env):
         type1 = self.operand1.infer_type(env)
@@ -9666,17 +9669,13 @@ class BoolBinopNode(ExprNode):
             is_temp=self.is_temp)
 
     def coerce_to(self, dst_type, env):
-        node = BoolBinopNode.from_node(
-            self,
-            operator=self.operator,
-            operand1=self.operand1.coerce_to(dst_type, env).coerce_to_simple(env),
-            operand2=self.operand2.coerce_to(dst_type, env).coerce_to_simple(env),
-            type=dst_type,
-            is_temp=self.is_temp)
         if not dst_type.is_pyobject:
-            if node.operand1.is_ephemeral() or node.operand2.is_ephemeral():
+            if self.operand1.is_ephemeral() or self.operand2.is_ephemeral():
                 error(self.pos, "Unsafe C derivative of temporary Python reference used in and/or expression")
-        return node
+
+        return GenericBoolBinopNode.from_node(
+            self, env=env, type=dst_type,
+            operator=self.operator, operand1=self.operand1, operand2=self.operand2)
 
     def analyse_types(self, env):
         # Note: we do not do any coercion here as we most likely do not know the final type anyway.
@@ -9734,8 +9733,8 @@ class BoolBinopNode(ExprNode):
     def generate_operand1_test(self, code):
         #  Generate code to test the truth of the first operand.
         if self.type.is_pyobject:
-            test_result = code.funcstate.allocate_temp(PyrexTypes.c_bint_type,
-                                                       manage_ref=False)
+            test_result = code.funcstate.allocate_temp(
+                PyrexTypes.c_bint_type, manage_ref=False)
             code.putln(
                 "%s = __Pyx_PyObject_IsTrue(%s); %s" % (
                     test_result,
@@ -9746,6 +9745,122 @@ class BoolBinopNode(ExprNode):
         return (test_result, self.type.is_pyobject)
 
 
+class BoolBinopResultNode(ExprNode):
+    subexprs = ['arg', 'value']
+    is_temp = True
+    arg = None
+    value = None
+
+    def __init__(self, arg, result_type, env):
+        arg = ProxyNode(arg)  # in case something wants to replace it later
+        super(BoolBinopResultNode, self).__init__(
+            arg.pos, arg=arg, type=result_type,
+            value=CloneNode(arg).coerce_to(result_type, env).coerce_to_simple(env))
+
+    def generate_operand_test(self, code):
+        #  Generate code to test the truth of the first operand.
+        if self.arg.type.is_pyobject:
+            test_result = code.funcstate.allocate_temp(
+                PyrexTypes.c_bint_type, manage_ref=False)
+            code.putln(
+                "%s = __Pyx_PyObject_IsTrue(%s); %s" % (
+                    test_result,
+                    self.arg.py_result(),
+                    code.error_goto_if_neg(test_result, self.pos)))
+        else:
+            test_result = self.arg.result()
+        return (test_result, self.arg.type.is_pyobject)
+
+    def generate_bool_evaluation_code(self, code, final_result_temp, and_label, or_label, end_label):
+        code.mark_pos(self.pos)
+
+        # x => x
+        # x and ... or ... => next 'and' / 'or'
+        # False ... or x => next 'or'
+        # True and x => next 'and'
+        # True or x => True (operand)
+
+        self.arg.generate_evaluation_code(code)
+        if and_label or or_label:
+            test_result, uses_temp = self.generate_operand_test(code)
+            sense = '!' if or_label else ''
+            code.putln("if (%s%s) {" % (sense, test_result))
+            if uses_temp:
+                code.funcstate.release_temp(test_result)
+            self.arg.generate_disposal_code(code)
+
+            if or_label:
+                # value is false => short-circuit to next 'or'
+                code.put_goto(or_label)
+                code.putln("} else {")
+            if and_label:
+                # value is true => go to next 'and'
+                code.put_goto(and_label)
+                if not or_label:
+                    code.putln("} else {")
+
+        if not and_label or not or_label:
+            # if no next 'and' or 'or', we provide the result
+            self.value.generate_evaluation_code(code)
+            self.value.make_owned_reference(code)
+            code.putln("%s = %s;" % (final_result_temp, self.value.result()))
+            self.value.generate_post_assignment_code(code)
+            self.arg.generate_disposal_code(code)
+            self.value.free_temps(code)
+            if and_label or or_label:
+                code.put_goto(end_label)
+
+        if and_label or or_label:
+            code.putln("}")
+        self.arg.free_temps(code)
+
+
+class GenericBoolBinopNode(BoolBinopNode):
+    """
+    BoolBinopNode with arbitrary non-bool result type.
+    """
+    subexprs = ['operand1', 'operand2']
+    is_temp = True
+
+    def __init__(self, pos, env, type, operator, operand1, operand2, **kwargs):
+        super(GenericBoolBinopNode, self).__init__(
+            pos, operator=operator, type=type,
+            operand1=self._wrap_operand(operand1, type, env),
+            operand2=self._wrap_operand(operand2, type, env),
+            **kwargs)
+
+    def _wrap_operand(self, operand, result_type, env):
+        if isinstance(operand, (GenericBoolBinopNode, BoolBinopResultNode)):
+            return operand
+        if isinstance(operand, BoolBinopNode):
+            return operand.coerce_to(result_type, env)
+        else:
+            return BoolBinopResultNode(operand, result_type, env)
+
+    def generate_bool_evaluation_code(self, code, final_result_temp, and_label, or_label, end_label):
+        code.mark_pos(self.pos)
+
+        outer_labels = (and_label, or_label)
+        if self.operator == 'and':
+            my_label = and_label = code.new_label('next_and')
+        else:
+            my_label = or_label = code.new_label('next_or')
+        self.operand1.generate_bool_evaluation_code(code, final_result_temp, and_label, or_label, end_label)
+
+        and_label, or_label = outer_labels
+
+        code.put_label(my_label)
+        self.operand2.generate_bool_evaluation_code(code, final_result_temp, and_label, or_label, end_label)
+
+    def generate_evaluation_code(self, code):
+        self.allocate_temp_result(code)
+        or_label = and_label = None
+        end_label = code.new_label('bool_binop_done')
+        self.generate_bool_evaluation_code(code, self.result(), and_label, or_label, end_label)
+        if code.label_used(end_label):
+            code.put_label(end_label)
+
+
 class CondExprNode(ExprNode):
     #  Short-circuiting conditional expression.
     #