GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — GH-106008: Make implicit boolean conversions explicit by brandtbucher · Pull Request #106003 · python/cpython Conversation
|
| } | ||
|
|
||
| inst(TO_BOOL_BOOL, (unused/1, unused/2, value -- value)) { | ||
| // Coolest (and dumbest-named) specialization ever: |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
True, but the not the most useful comment for someone trying to understand the code 🙂
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| inst(TO_BOOL_NONE, (unused/1, unused/2, value -- res)) { | ||
| // This one is a bit weird, because we expect *some* failures... | ||
| // it might be worth combining with TO_BOOL_ALWAYS_TRUE somehow: |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think we decided it wasn't, as it reflects the underlying type instability when doing if x: as a stand in for if x is None.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| inst(TO_BOOL_STR, (unused/1, unused/2, value -- res)) { | ||
| DEOPT_IF(!PyUnicode_CheckExact(value), TO_BOOL); | ||
| STAT_INC(TO_BOOL, hit); | ||
| if (Py_Is(value, &_Py_STR(empty))) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Use value == &_Py_STR(empty). The semantics is value == "", not value is "".
In general I wouldn't use Py_Is except when you want the exact semantics of Python's x is y.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Hm, I mean, we are checking for the identity of the singleton string here. I'll just change it, though.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Hypothetically we could have more than one "" object. I don't think that " ".strip() is "" is part of the language spec, so Py_Is doesn't add any safety, just obfuscation.
We can also potentially have tagged ints, in which case Py_Is would need to become a lot more complex, but value == &_Py_STR(empty) would remain efficient.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| inst(TO_BOOL_ALWAYS_TRUE, (unused/1, version/2, value -- res)) { | ||
| // This one is a bit weird, because we expect *some* failures... | ||
| // it might be worth combining with TO_BOOL_NONE somehow: |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
See comment above.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
| } | ||
| assert(PyBool_Check(cond)); | ||
| JUMPBY(oparg * Py_IsFalse(cond)); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This is so much more pleasing 🙂
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| err += add_stat_dict(stats, UNPACK_SEQUENCE, "unpack_sequence"); | ||
| err += add_stat_dict(stats, FOR_ITER, "for_iter"); | ||
| err += add_stat_dict(stats, TO_BOOL, "to_bool"); | ||
| err += add_stat_dict(stats, SEND, "send"); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I tend to forget about the stats dict .
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Note for possible future PR: We could, at the cost of two bits in Rather than check the version number, check the For abi4, we could add a per-object bit to handle immutable objects like ints and strings. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
| } | ||
| Py_DECREF(newconst); | ||
| return index; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
windows compiler warning here, implicit cast from Py_ssize_t to int
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Merging is currently blocked on #106250. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Hey @brandtbucher, I have a question about this PR. In #106393 I had to change the code in to The reason is that the uop executor currently exits whenever it jumps, and your original code from this PR always jumps. Did you (or @markshannon) have a reason to prefer the If there's no deep reason I'll keep it the way I coded it up; but if there is (maybe it came out faster in a micro-benchmark?) then I suppose I could fix it another way in the uop interpreter (e.g. only exiting if the jump offset is nonzero). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
That the implementation of branches is itself branchless has a certain aesthetic appeal 🙂. TBH, that's the main reason. The multiplication form will be quicker for unpredictable branches, and slower for predictable ones in the tier 1 interpreter. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Summary: Implements what we need from [GH-106008](python/cpython#106003). Technically the included updates to `UNARY_NOT`, and `POP_JUMP_IF_FALSE|TRUE` are optimizations and not strictly needed. However, it really makes sense to to them. Reviewed By: DinoV Differential Revision: D80682012 fbshipit-source-id: 6150d52dddd28ef5c2e401ed17df7c7cad09074b
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.