gh-117270: Add missed change to COMPARE_OP from 3.12 to dis documentation #117272
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
GH-117274 is a backport of this pull request to the 3.12 branch. |
…dis documentation (pythonGH-117272)
| @@ -1239,6 +1239,9 @@ iterations of the loop. | |||
| ``cmp_op[opname >> 5]``. If the fifth-lowest bit of ``opname`` is set | |||
| (``opname & 16``), the result should be coerced to ``bool``. | |||
|
|
|||
| .. versionchanged:: 3.12 | |||
| The cmp_op index is now stored in the four-highest bits of oparg instead of the four-lowest bits of oparg. | |||
There was a problem hiding this comment.
I don't believe this is true. The index is not stored in the most significant bits of oparg and to be honest it's not easy to define most significant bits in Python's integer - it's not fixed length.
There was a problem hiding this comment.
And I don't think the index is ever stored in the four lowest bits - that indicates we have 16 (or at least 9) operations.
There was a problem hiding this comment.
Notice that even for 3.13, it did not mention the version change for the operation name shift. I think we can just leave it there and just fix the 3.12 docs.
There was a problem hiding this comment.
For opcodes, the arg is a byte unless prefixed by EXTENDED_ARG; for 3.13, it the top 3 bits (hence the shift by 5), for 3.12, it the top 4 bits (with the MSB always 0, and hence, the shift by 4).
No strong opinion on including/excluding the versionchanged; I prefer having it as a record so people looking at the latest docs know code that a breaking change occurred on 3.12 for COMPARE_OP.
There was a problem hiding this comment.
And that's a C-level implementation detail that's not exposed or documented. "Top X bits" only works when the variable is fixed-size, and when we are discussing in Python scope, opname is not fixed-size because it's an int. Even in C code, oparg is often declared as int or uint16_t in functions, and it would be super confusing when "top X bits" means the top X bits when it is a byte. I would assume that's why the documentation below used "the fifth-lowest bit" instead of "the fourth-highest bit" - counting from lowest is always well defined, but counting from the highest is not.
There was a problem hiding this comment.
Would "The cmp_op index now starts at the fifth-lowest bit of oparg." be a better wording?
There was a problem hiding this comment.
"start" is also ambiguous in my opinion. It's not clear whether the data goes "high" or "low" starting from a specific position. (That's why I think cmp_op[opname >> 5] is the best way to describe the fact as it is).
If we have to have a version-changed note, I would prefer something like "The cmp_op index is stored in bit(LSB maybe?) 7 to bit 4" or "The cmp_op index is stored in 8th lowest bit to 5th lowest bit". I believe the index is actually a 3-bit value instead of 4 but that might not be a big issue here. But we do have a 0-index vs 1-index issue.
There was a problem hiding this comment.
Maybe "the cmp_op index is now shifted 4 bits to the left"?
📚 Documentation preview 📚: https://cpython-previews--117272.org.readthedocs.build/