-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
gh-142168: explicitly initialize stack_array in _PyEval_Vector and _PyEvalFramePushAndInit_Ex
#142192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
See my comment on the issue. There is prior art and initializing in this hot path to silence a false positive warning is not (my) preferred solution. |
|
Initialization in a hot path is not desired and it's also a false positive. I'm going to close this PR, sorry. |
|
IMO this change is a reasonable fix for the warnings. |
|
I think we should have checked the produced assembly to see whether this introduces a true overhead in an optimized build. Personally, I think the pragmas are fine (we do this elsewhere) but we could disable them in |
|
Well, we can check and then we know for this compiler and version we've checked. And are at the mercy of all compilers and versions that they hopefully behave the same - i.e. do not produce needless initialization code we've put in to silence a false positive warning. |
I think so, too. It is good to have less warnings. It is even better to have less false positive warnings :) |
|
OTOH, Footnotes
|
|
Reopening per my comment above ... |
|
If we want pragmas we have the other PR which should be preferred over this one. It we want an explicit initialization we can use this one. OTOH while it is a false positive it may become a non-false positive if we were to change the code a bit so it may be worth doing the explicit initialiaztion. |
vstinner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. IMO it's a reasonable fix to avoid warnings about uninitialized variables.
|
As said I am fine either way and happy when the warning is gone. Leaving up to @markshannon or @Fidget-Spinner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_PyEvalFramePushAndInit_Ex is only used by _DO_CALL_FUNCTION_EX. That uop already constructs a tuple args and ocassionally dict kwargs. This is thus nothing in comparison to those. We should just merge it. Any effect on performance is likely not even measurable.
|
Actually, the commit is not by this author? EDIT: oh this is just an alt account, looks like the same person. |
|
@Fidget-Spinner What about _PyEval_Vector? |
stack_array in _PyEval_Vector and _PyEvalFramePushAndInit_Ex
|
_PyEval_Vector is a little trickier. It's actually in the hot path of vectorcalls to Python functions in C-to-Python calls. However, again I suspect the cost of this is so low with respect to everything else (frame creation and all that) it doesnt matter. |
|
Let's merge this and if there is a noticeable performance loss, then people will complain :') |
To stop the warning.
Reproduce steps: