double init filename_obj value in pythonrun.c function PyRun_AnyFileExFlags

[gorsing]

Bug report

Bug description:

/* Parse input from a file and execute it */
int
PyRun_AnyFileExFlags(FILE *fp, const char *filename, int closeit,
                     PyCompilerFlags *flags)
{
    PyObject *filename_obj;
    if (filename != NULL) {
        filename_obj = PyUnicode_DecodeFSDefault(filename);
        if (filename_obj == NULL) {
            PyErr_Print();
            return -1;
        }
    }
    else {
        filename_obj = NULL;
    }
    int res = _PyRun_AnyFileObject(fp, filename_obj, closeit, flags);
    Py_XDECREF(filename_obj);
    return res;
}

===================== more better =============

/* Parse input from a file and execute it */
int
PyRun_AnyFileExFlags(FILE *fp, const char *filename, int closeit,
                     PyCompilerFlags *flags)
{
    PyObject *filename_obj = NULL;
    if (filename != NULL) {
        filename_obj = PyUnicode_DecodeFSDefault(filename);
        if (filename_obj == NULL) {
            PyErr_Print();
            return -1;
        }
    }
    int res = _PyRun_AnyFileObject(fp, filename_obj, closeit, flags);
    Py_XDECREF(filename_obj);
    return res;
}

CPython versions tested on:

3.8, 3.9, 3.10, 3.11, 3.12, 3.13, CPython main branch

Operating systems tested on:

Linux, macOS, Windows, Other

Linked PRs

• gh-115914: double use init filename_obj value in pythonrun.c function PyRun_AnyFileExFlags #115916

[sobolevn]

Hi, thanks a lot for the suggestion 👍

This is not really a bug, this is more like a refactoring opportunity.
I think it is worth doing when somebody will touch this function for a different reason (I don't think that this will happen soon enough).

But, pure stylistic changes are not-a-so-good idea, because they can introduce new bugs, damage git history and does not bring much value.

Let's close this issue for now: because everything works just fine :)

[gorsing]

Hello! Thank you very much for your quick feedback👍

This may actually be a very small bug, but the refactoring option would still be a good idea. This change does not change the behavior of the feature, and initial tests did not reveal any problems.

Improvement:

Simplifying the code. This change makes the code easier to understand because filename_obj is guaranteed to have an initial value, eliminating the need to check for that value in a separate else block. This makes the code cleaner and easier to read.

Reduced chance of error. Initializing variables before using them is good programming practice. This helps prevent errors caused by using uninitialized variables, which could lead to undefined behavior.

Efficiency: Although the change seems minor from a performance perspective, it does improve coding style by ensuring that all variables are properly initialized before use. In this case, the modification is more about code readability and maintainability than a direct performance increase.

[gpshead]

FWIW - I accepted the PR in this specific case regardless as it was trivial and an obvious coding pattern modernization.

In general we don't have a process to handle this kind of thing well. One issue per example of this would be a millions issues for our old code base. But one issue going around cleaning all of these up in large swaths would also be frowned upon as unnecessary churn adding low value change reviewer burden and increasing backporting difficulty for others. 🤷

Thanks for the little contribution, though please understand we may not always bother to accept this kind of trivia.

[gorsing]

@gpshead Thank you for your time.