GH-145247: Implement _PyTuple_FromPair#145325
GH-145247: Implement _PyTuple_FromPair#145325sergey-miryanov wants to merge 6 commits intopython:mainfrom
Conversation
eendebakpt
left a comment
eendebakpt
left a comment
There was a problem hiding this comment.
To minor comments on the test, but overall this looks good!
| def test_tuple_from_pair(self): | ||
| # Test _PyTuple_FromPair, _PyTuple_FromPairSteal | ||
| ctors = (("_PyTuple_FromPair", _testinternalcapi._tuple_from_pair), | ||
| ("_PyTuple_FromPairSteal", _testinternalcapi._tuple_from_pair_steal)) |
There was a problem hiding this comment.
The _testinternalcapi._tuple_from_pair_steal that wraps _PyTuple_FromPairSteal increments references to the arguments. For that reason there is no way to make a distinction between the _testinternalcapi._tuple_from_pair and _testinternalcapi._tuple_from_pair._tuple_from_pair_steal in the tests)
I would either change the name of _tuple_from_pair_steal or add a comment to make this clear.
There was a problem hiding this comment.
I'm not sure I understand what you mean. Could you elaborate please?
You are right that tests seem equals from point of view, but we need to test both functions to check a) their behavior and b) absence of the leak if we run it with -R option.
There was a problem hiding this comment.
The test is good indeed.
What I meant is that _testinternalcapi._tuple_from_pair_steal is not stealing references (since the arguments are incremented before passing to _PyTuple_FromPairSteal. So a (way too long) descriptive name would be _testinternalcapi._wraps_tuple_from_pair_steal_but_does_not_actually_steal.
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
| for name, ctor in ctors: | ||
| with self.subTest(name): | ||
| self.assertEqual(type(ctor(1, 2)), tuple) | ||
| self.assertEqual(ctor(1, 2), (1, 2)) |
There was a problem hiding this comment.
| self.assertEqual(ctor(1, 2), (1, 2)) | |
| self.assertEqual(ctor(1, 145325), (1, 145325)) |
The series of asserts contained only immortal elements for the tuple. Changing 2 to a semi-random higher number that is not part of the cpython small ints.
2 participants
PyTuple_FromPair#145247