Repository navigation
gh-139393: fix _CALL_LEN JIT tests for tuples - #139394
Conversation
picnixz
added
skip news
and removed
tests
ashm-dev
left a comment
ashm-dev
left a comment
There was a problem hiding this comment.
Everything is ok, I tested the fixes on clang and gcc works without problems!)
| if (PyModule_AddIntMacro(module, _PY_NSMALLNEGINTS) < 0) { | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
This isn't used anywhere, is it?
There was a problem hiding this comment.
Mmh, yes, this one isn't used anymore. I wondered whether I needed one but I don't need to. I'll amend this one.
There was a problem hiding this comment.
Side comment: this function should return -1 instead of 1 as it's the case for other Py_mod_exec functions (I got surprised by the "return 1"). It doesn't really matter per se, but I think we should generally document what kind of values we expect for Py_mod_* handlers and tp_* ones (it's not clear what tp_clear should return and when).
Currently, errors in Py_mod_exec functions are indicated by a non-zero return value, but it's not documented. I would advise that we document this.
| def testfunc(n): | ||
| class C: | ||
| t = tuple(range(300)) | ||
| t = tuple(range(2048)) |
There was a problem hiding this comment.
Instead of using a hardcoded magic number, it would be nice if we used _PY_NSMALLPOSINTS * 2 or something like that to make sure this test always works, even if we increase _PY_NSMALLPOSINTS in the future.
There was a problem hiding this comment.
I am actually unsure whether we need hardcoding here actually because of the JIT. I'll check if changing it to a compile-time known value is necessary or not.
There was a problem hiding this comment.
Yeah that's what I feared. While the test still passes, the uops will be different. I don't remember when we have some inlined computations x * y that are then transformed in a single LOAD_CONST.
There was a problem hiding this comment.
Also, I think we should make such change at other places as well. If you want to, let's open a separate issue.
4 participants
This fixes a regression introduced in 7ce25ed when
_PY_NSMALLPOSINTSwas changed from 257 to 1025.test_capi.test_opt.test_call_len_known_lengthfails on JIT+debug builds #139393