bpo-36328: Fix compiler warning in Py_NewInterpreter()

[matrixise]

https://bugs.python.org/issue36328

[ericsnowcurrently]

The code in new_interpreter() means this change is technically unnecessary. However, it's innocuous and mostly correct. The compiler is probably thrown off by how "fatal" errors are handled in the "init failed" cases.

Regardless, I recommend adding *tstate = NULL; at the beginning of new_interpreter() instead of in the various failure cases. With that you shouldn't need to change Py_NewInterpreter(). It will also benefit any other call sites.

[bedevere-bot]

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[ericsnowcurrently]

FWIW, a related remaining issue I have with new_interpreter() is that it doesn't guarantee an error is set when tstate is "returned" as NULL (i.e. most of the return _Py_INIT_OK(); cases). That isn't a problem in the existing code. However, it would be easy to miss that when adding a non-fatal error case.

That said, it isn't that big a deal and you don't need to address it in this PR (or at all). :)

However, it may be worth considering fixing here or opening a separate issue. At very least a comment would help. Dealing with it in the handle_error goto label would benefit all call sites. That could be done with assert PyErr_Occurred() or by PyErr_SetString(PyExc_RuntimeError, "..."), etc.

[matrixise]

@ericsnowcurrently when adding *interp = NULL in new_interpreter and remove the *interp = NULL in Py_NewInterpreter I get the same warning from the compiler.

diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c
index 49a2f18e49..0c9ab4a95e 100644
--- a/Python/pylifecycle.c
+++ b/Python/pylifecycle.c
@@ -1276,7 +1276,7 @@ static _PyInitError
 new_interpreter(PyThreadState **tstate_p)
 {
     PyInterpreterState *interp;
-    PyThreadState *tstate, *save_tstate;
+    PyThreadState *tstate = NULL, *save_tstate;
     PyObject *bimod, *sysmod;
     _PyInitError err;
 
@@ -1434,7 +1434,7 @@ handle_error:
 PyThreadState *
 Py_NewInterpreter(void)
 {
-    PyThreadState *tstate = NULL;
+    PyThreadState *tstate;
     _PyInitError err = new_interpreter(&tstate);
     if (_Py_INIT_FAILED(err)) {
         _Py_ExitInitError(err);

Python/pylifecycle.c: Dans la fonction « Py_NewInterpreter »:
Python/pylifecycle.c:1442:12: warning: « tstate » pourrait être utilisé sans être initialisé dans cette fonction [-Wmaybe-uninitialized]
     return tstate;
            ^~~~~~

[vstinner]

LGTM.

[vstinner]

The code in new_interpreter() means this change is technically unnecessary.

Sadly, compiler are weak static analyzers which commonly emit false alarms. Adding "= NULL" doesn't hurt performance but allow to focus on real bugs: real warnings, by ignoring such false alarm.

Regardless, I recommend adding *tstate = NULL; at the beginning of new_interpreter() instead of in the various failure cases. With that you shouldn't need to change Py_NewInterpreter(). It will also benefit any other call sites.

I see multiple cases where new_interpreter() returns NULL on purpose. I have no opinion on this API. At least, the code didn't change since Python 3.6: it's not a a regression my recent changes using _PyInitError.

FWIW, a related remaining issue I have with new_interpreter() is that it doesn't guarantee an error is set when tstate is "returned" as NULL (i.e. most of the return _Py_INIT_OK(); cases).

Honestly, I don't understand Py_NewInterpreter() API. I don't understand why some errors are fatal, but some others are not. Sadly, the current Py_NewInterpreter() API can only return NULL to indicate an error... and the exception is not passed to the caller: "handle_error:" label prints the exception and clears it...

That isn't a problem in the existing code. However, it would be easy to miss that when adding a non-fatal error case. That said, it isn't that big a deal and you don't need to address it in this PR (or at all). :) However, it may be worth considering fixing here or opening a separate issue. At very least a comment would help. Dealing with it in the handle_error goto label would benefit all call sites. That could be done with assert PyErr_Occurred() or by PyErr_SetString(PyExc_RuntimeError, "..."), etc.

Maybe things can be done, but IMHO this PR is just fine: it does what it says, fix a compiler warning :-)