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

Improved (in)equality testing with non-boolean results #5533

Open
wants to merge 10 commits into
base: master
from

Conversation

@nickovs
Copy link
Contributor

nickovs commented Jan 13, 2020

This is a version of #5479 which makes use of a new flag in the flags word of mp_obj_type_t to indicate if various shortcuts can not be used when performing equality and inequality tests, as per the comment in the discussion of that PR.

The semantics of the new TYPE_FLAG_NO_EQUALITY_SHORTCUTS are that when this is clear (or uninitialised, in a static structure) the code can assume that this class (a) only ever returns a boolean result, (b) is reflexive, (c) only implements the __eq__ operator and not the __ne__ operator and (d) can not be equal to an instance of any different class that also has this flag clear. If the flag is set then at least one of these assumptions does not hold.

Currently four built-in classes have the flag set: float and complex are non-reflexive (since nan != nan) while bytearray and frozenszet instances can equal other builtin class instances (bytes and set respectively).

The flag is set for any new class defined by the user. In the future, code could be added to analyse user-defined classes to see if either of the __eq__ or __ne__ special methods are defined and clear the flag if both are absent in the class and all its superclasses.

Adding the flag (and setting it for a few built-in classes) looks like it reduced the amount of code and it should now take the fast paths for most common cases, while further improving the compatibility with CPython behaviour.

The PR also expands the test cases to cover some cases that previously did not function as CPython did.

@dpgeorge dpgeorge added the py-core label Jan 13, 2020
@dpgeorge

This comment has been minimized.

Copy link
Member

dpgeorge commented Jan 13, 2020

Thanks for following up!

Initial tests for size difference gives:

   bare-arm:  +108 +0.164% 
minimal x86:  +220 +0.149% 
   unix x64:  +112 +0.022% 
unix nanbox:   +32 +0.007% 
      stm32:   +92 +0.024% PYBV10
     cc3200:  +120 +0.065% 
    esp8266:   +64 +0.009% GENERIC
      esp32:    -8 -0.001% GENERIC[incl -32(data)]
        nrf:   +76 +0.052% pca10040
       samd:   +96 +0.095% ADAFRUIT_ITSYBITSY_M4_EXPRESS

That's very reasonable.

py/objtype.h Outdated Show resolved Hide resolved
@nickovs

This comment has been minimized.

Copy link
Contributor Author

nickovs commented Jan 15, 2020

@dpgeorge Something strange is happening with the test coverage. The new tests that I just added do cover the one line in objtype.c that shows as uncovered when I run make coverage_test on my Linux machine. I don't see how the tests could complete without exercising this path and I don't see how that the compiler could ever optimise this away. If you get the chance to run the coverage tests perhaps you could look to see if line 1398 in objtype.c is being hit.

@dpgeorge

This comment has been minimized.

Copy link
Member

dpgeorge commented Jan 15, 2020

The new tests that I just added do cover the one line in objtype.c that shows as uncovered when I run make coverage_test on my Linux machine.

Yeah, that happens sometimes. It seems that gcov is not always accurate with the lines it hits. I'll double check it though.

@dpgeorge

This comment has been minimized.

Copy link
Member

dpgeorge commented Jan 24, 2020

I see that the check and warning for comparison between str and bytes is now gone. There is a test for this in tests/basics/bytes_compare3.py but, because enabling the warning is a compile-time option, the test will actually pass even if the warning is not printed.

IMO the check and warning should be retained. I don't think there's an elegant way to add it, I think there needs to be explicit tests in mp_obj_equal_bop() for it (similar to how it worked before).

@nickovs

This comment has been minimized.

Copy link
Contributor Author

nickovs commented Jan 25, 2020

I've reinstated the warning for comparisons of strings and bytes.

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.