| msg126306 - (view) |
Author: Devin Jeanpierre (Devin Jeanpierre) * |
Date: 2011-01-15 01:00 |
| PyObject_RichCoareBool is, according to the documentation in trunk (Doc\c-api\object.rst), exactly the same as PyObject_RichCompare, except it 1, 0, or -1 for error, false, and true respectively. However, it differs in behavior by both that, and also by assuming that identity implies equality. This noted in a two year-old ML post (sadly, no bug report was posted as best as I can find): http://mail.python.org/pipermail/python-list/2009-March/1195170.html Ideally PyObject_RichCompareBool should probably be named something else, since it can no longer be used, strictly, as "PyObject_RichCompare, but returning a C bool" (or, rather, a C int). Some suggestions were offered in the mailing list thread above. I'm filing this as a documentation bug because I find that outcome unlikely. At least the documentation should note the difference in behavior, so that people do not accidentally write C code that does not behave as intended. This issue is related to, but different from issue 4296, which objected to the new container behavior created by the change to PyObject_RichCompareBool. My only objection here is that the latter change does not appear to be documented. I would supply a patch for the tests, but PyObject_RichCompareBool is apparently not directly tested anywhere, just tested obliquely through testing the containment operator, and this was changed by the same commit that changed PyObject_RichCompareBool. I don't know how to word the (very small!) change to the docs. |
|
|
| msg126320 - (view) |
Author: Eli Bendersky (eli.bendersky) *  |
Date: 2011-01-15 09:00 |
| I agree that the difference in behavior between the two functions is unfortunate, but that's unlikely to change now. The least we can do is make the documentation precise. I'm attaching a proposed patch to Doc/c-api/object.rst Nick/Terry/Georg - if this looks OK I can commit. |
|
|
| msg126321 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2011-01-15 09:17 |
| Sure. |
|
|
| msg126322 - (view) |
Author: Eli Bendersky (eli.bendersky) * |
Date: 2011-01-15 10:23 |
| Committed the fix to py3k in r88009 |
|
|
| msg126323 - (view) |
Author: Eli Bendersky (eli.bendersky) * |
Date: 2011-01-15 10:35 |
| Backport to release31 branch in r88010 |
|
|
| msg126330 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2011-01-15 14:32 |
| For the record, the gory details as to *why* RichCompareBool makes more assumptions as to the meaning of equality than the basic RichCompare function can be found in issue 4296 (I just found that issue myself by looking at Mark's response to the python-list thread). It may be worth explicitly pointing out that use cases where this assumption is unacceptable would be better served by direct invocation RichCompare function. |
|
|
| msg126331 - (view) |
Author: Eli Bendersky (eli.bendersky) * |
Date: 2011-01-15 14:37 |
| Nick, I read the protocol of issue 4296 and I understand the reasoning behind the code, but I still think the naming is mightily confusing. Two distinct changes went into a single function (A) Return -1/0/+1 instead of PyObject and (B) the id() shortcut, and its name is inappropriate. Were an API change feasible, some other naming would be better. > "It may be worth explicitly pointing out that use cases where this assumption is unacceptable would be better served by direct invocation RichCompare function." Do you mean write it down in the docs? |
|
|
| msg126332 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2011-01-15 14:50 |
| Yeah, to prevent perfectly reasonable "why" questions, it is probably worth providing a little extra justification as an addendum to your new note (which is already an improvement on the complete silence on the topic that existed before). A possible addition: "... This assumption allows invariants such as "x in [x]" to be more easily guaranteed by the interpreter. If the assumption is not valid for a given use case, call PyObject_RichCompare() directly instead of using this function." For 3.3, it *may* make sense to provide a PyObject_RichCompareBoolEx() function which includes an additional "reflexive" parameter. Then the existing PyObject_RichCompareBool() semantics would just be the new function with the reflexive argument set to 1. |
|
|
| msg126350 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2011-01-15 23:23 |
| -1 on PyObject_RichCompareBoolEx() for 3.3 -- it is simply an invitation to shoot yourself (or others) in the foot. I've not seen real world code using bool(a==b) or its C equivalent, so it's hard to believe that there is a use case here (unless you're trying to commit atrocities with NaN-like objects). |
|
|
| msg126352 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2011-01-15 23:52 |
| Indeed. I was actually wondering if it would be worth trying to write up a section for the language reference to describe the cases where a Python implementation is *expected* to assume reflexive equality. We (IMO) have a problem at the moment due to situations like: >>> class PyContains(list): ... def __contains__(self, obj): ... return any(x==obj for x in self) ... >>> nan = float("nan") >>> nan in [nan] True >>> nan in PyContains([nan]) False This is a bug in the __contains__ definition (it should use "x is obj or x == obj" rather than just the latter expression) but there isn't anything in the language reference to point that out. Assuming reflexive equality in some places and not in others based on the underlying implementation language is going to be a source of subtle bugs relating to types like float and decimal.Decimal. |
|
|
| msg126356 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2011-01-16 00:50 |
| If something needs to be written about NaNs or other invariant destroying objects, perhaps a FAQ entry would suffice (perhaps referencing http://bertrandmeyer.com/2010/02/06/reflexivity-and-other-pillars-of-civilization/ ). |
|
|
| msg126359 - (view) |
Author: Eli Bendersky (eli.bendersky) * |
Date: 2011-01-16 05:43 |
| Raymond, I initially set "easy" on this issue because I considered it a documentation issue, not the place to resolve the harder debate of the semantics of these functions. Perhaps I was wrong? Also, I agree with Nick that the difference must be further clarified in "stronger words". If there's no debate on this issue, I will commit a fix after the freeze is over. |
|
|
| msg126360 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2011-01-16 06:01 |
| Yes, it is a doc issue but sometimes those aren't easy to get right (in terms of being beneficial to the reader and in good alignment with the design intentions). Please attach the doc patch you want to go in and I'll review it sometime the freeze. |
|
|
| msg134554 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2011-04-27 12:19 |
| Reopening, as a 2.7 backport of this would be a nice thing to have. |
|
|
| msg134837 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2011-04-30 05:53 |
| New changeset d27f95e3b52f by Eli Bendersky in branch '2.7': Issue #10912: add clarification for PyObject_RichCompareBool comparing identical objects http://hg.python.org/cpython/rev/d27f95e3b52f |
|
|