Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds equality operator for booleans #489

Open
wants to merge 6 commits into
base: develop
from

Conversation

@petscheit
Copy link
Contributor

commented Oct 2, 2019

implements #483

Copy link
Member

left a comment

Nice and lean addition, however I think it currently does not behave as expected. Let's add a test for false == false which should return true.

BooleanExpression::EqBool(box lhs, box rhs) => self.flatten_boolean_expression(
symbols,
statements_flattened,
BooleanExpression::And(box lhs.clone(), box rhs.clone()),

This comment has been minimized.

Copy link
@Schaeff

Schaeff Oct 2, 2019

Member

Isn't this incorrect? false && false == false but (false == false) == true
Also the cloning sounds unnecessary here.

One ok way to do this would be to use the same flattening as for BooleanExpression::Eq below, though with Booleans we have the guarantee that they flatten to either 0 or 1, so maybe this extra guarantee makes a cheaper check possible. The Field one is 2 constraints right now so it would need to be 1 constraint to be better I suppose?

This comment has been minimized.

Copy link
@petscheit

petscheit Oct 4, 2019

Author Contributor

I agree, wasn't thought through enough from my side.

   BooleanExpression::BoolEq(box lhs, box rhs) => self.flatten_boolean_expression(
               symbols,
               statements_flattened,
               BooleanExpression::Or(
                   box BooleanExpression::And(box lhs.clone(), box rhs.clone()), 
                   box BooleanExpression::And(
                       box BooleanExpression::Not(box lhs), 
                       box BooleanExpression::Not(box rhs)
                   ),
               )
           ),

Something like this should work functionally right, though I'm not sure how efficient this is. I don't think I understand the flattening part of this comment. What do you mean by the Field one?

This comment has been minimized.

Copy link
@Schaeff

Schaeff Oct 6, 2019

Member

I meant "equality for field elements" as implemented in flatten.rs for BooleanExpression::Eq.

I'm not sure how efficient this is

You could give it a try with a simple program:

def main(bool a, bool b) -> (bool):
   return a == b

Unless the optimiser does some nice magic, it does look inefficient

@@ -513,6 +513,10 @@ pub enum BooleanExpression<'ast, T: Field> {
Box<FieldElementExpression<'ast, T>>,
Box<FieldElementExpression<'ast, T>>,
),
EqBool(

This comment has been minimized.

Copy link
@Schaeff

Schaeff Oct 2, 2019

Member

I think for consistency, if we call this EqBool we should rename Eq to EqField.

I also have a preference for BoolEq like "boolean equality".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.