gh-131798: JIT: Narrow the return type of _GET_LEN to int
#133345
Conversation
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Signed-off-by: Manjusaka <[email protected]>
Signed-off-by: Manjusaka <[email protected]>
Signed-off-by: Manjusaka <[email protected]>
Signed-off-by: Manjusaka <[email protected]>
Signed-off-by: Manjusaka <[email protected]>
Signed-off-by: Manjusaka <[email protected]>
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @Fidget-Spinner, @brandtbucher: please review the changes made to this pull request. |
|
BTW @brandtbucher @Fidget-Spinner sorry about the force push. I set pull with rebase as default on my environment. I will modify it in next PR( |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Signed-off-by: Manjusaka <[email protected]>
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @brandtbucher, @Fidget-Spinner: please review the changes made to this pull request. |
Signed-off-by: Manjusaka <[email protected]>
Lib/test/test_capi/test_opt.py
Outdated
| @@ -1925,6 +1925,34 @@ def testfunc(n): | |||
| self.assertNotIn("_GUARD_NOS_INT", uops) | |||
| self.assertNotIn("_GUARD_TOS_INT", uops) | |||
|
|
|||
| def test_get_len(self): | |||
There was a problem hiding this comment.
You are missing the test case that Brandt suggested: #133345 (comment)
You could also split test_get_len into two separate test cases - other test cases typically only have one testfunc :)
There was a problem hiding this comment.
I think we need two case here
match tuple, theGET_LENwill return a constmatch non-tuple, theGET_LENwill return a non-const lang
I have split this into two test case. Plz correct me if I'm wrong. PTAL when you get time
There was a problem hiding this comment.
If I understood correctly, Brandt was suggesting a test case with a non-constant tuple. That is, a tuple with non-constant values, which is different from a non-tuple. So for example (object(), object()) (non-constant tuple) rather than [1, 2] (non-tuple) 🙂
There was a problem hiding this comment.
If I understood correctly, Brandt was suggesting a test case with a non-constant tuple.
Yes, this is the original version of my thought. But I find that the code we add the the _GET_LEN will effect between tuple and non tuple. So I modify the test
But, your idea is making sense. Let me add test for non-constant tuple
Signed-off-by: Manjusaka <[email protected]>
Signed-off-by: Manjusaka <[email protected]>
Signed-off-by: Manjusaka <[email protected]>
Signed-off-by: Manjusaka [email protected]