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

Avoid conversion to int_ rhs argument of enum eq/ne #1912

Merged
merged 5 commits into from
Sep 19, 2019

Conversation

sizmailov
Copy link
Contributor

Fixes #1830

@wjakob
Copy link
Member

wjakob commented Sep 19, 2019

This change looks reasonable, but shouldn't it also be implemented consistently for the other operations (e.g. less than operator for arithmetic types, etc.)?

@sizmailov
Copy link
Contributor Author

I think the right way is to mock python behavior. Here is test snippet with int, string and custom types:

test snippet
class A(object): 
  def __repr__(self): return "A()"
class B(object): 
  def __repr__(self): return "B()"

def test_binop(op, f):
  try:
    result = f()
    print("`{} {} {}` = `{}`({})".format(repr(a),op,repr(b),result,type(result)))
  except Exception as e:
    print("`{} {} {}` raises {}: {}".format(repr(a),op,repr(b),type(e), e))

def test_unop(op, f):
  try:
    result = f()
    print("`{} {}` = `{}`({})".format(op,repr(a),result,type(result)))
  except Exception as e:
    print("`{} {}` raises {}: {}".format(op,repr(a),type(e), e))

for a,b in [(A(), B()),
            (1, 2), 
            (1, A()),
            (A(), 1),
            ]:
  test_binop("!=", lambda: a!=b)
  test_binop("==", lambda: a==b)
  test_binop("<", lambda: a<b)
  test_binop("<=", lambda: a<=b)
  test_binop(">", lambda: a>b)
  test_binop("&", lambda: a&b)
  test_binop("|", lambda: a|b)
  test_binop("^", lambda: a^b)

for a in [A(), 1, "string"]:
 test_unop("~", lambda: ~a)

snippet on repl.it: (python2) (python3)

A() < 1 is False in python2 and TypeError in python3.

Current behavior is in line with python3. I've updated tests to check behavior of enum arithmetic and behavior on operations with unsupported types.

Should we branch on python version or leave it as is?

While current behavior is in line with pytnon3, the exception message is a bit off-putting:

>       y ^ object()
E       TypeError: int() argument must be a string, a bytes-like object or a number, not 'object'

Additional check will duplicate checks inside of py::int_, so it's a bit of redundant work on non-exceptional path. extra try-catch feels like heavy-weight, but it should not affect non-exceptional path. Should it be added?

Enumeration elements may appear in docsting unordered
@sizmailov
Copy link
Contributor Author

For some reason enumeration elements order in docstring is not determined. Replaced exact docstring comparison with every-line-exists-in-docstring check.

@wjakob
Copy link
Member

wjakob commented Sep 19, 2019

I don't think the extra check is needed, and defaulting to Python 3 behavior sounds good to me.

@wjakob wjakob merged commit 09f0829 into pybind:master Sep 19, 2019
@sizmailov sizmailov deleted the fix_enum_compare branch October 8, 2019 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enum instance equality broken in pybind11 2.3
2 participants