Skip to content

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jun 25, 2023

Copy link
Contributor

@barneygale barneygale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for looking at this :)


func = _PyObject_LookupSpecial(o, &_Py_ID(__fspath__));
if (NULL == func) {
if ((NULL == func) || (func == Py_None)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few thoughts, no change requested:

  • The parentheses are technically redundant, but it's OK to keep them for extra clarity.
  • The NULL == func here is a "Yoda condition". People write that because C lets you write if (func = NULL), and it doesn't do what you want (it always sets func to NULL). The Yoda condition prevents this, because if (NULL = func) is probably a syntax error. However, Yoda conditions aren't that useful these days as compilers will warn about if (func = NULL). Arguably you're introducing an inconsistency here as one of the two conditions on this line is Yoda and the other isn't, but I think that's fine.
  • There is a Py_IsNone function (https://docs.python.org/3/c-api/structures.html#c.Py_IsNone) that could be used instead of == Py_None. I think the motivation is to allow alternate C APIs where Py_None might not be a singleton. I don't know if there is any push to use this function in CPython itself, though I see a few calls to it. However, there are already several == Py_None checks in this file, so it seems fine to add a few more.
  • I already mentioned this on Discord, but before 3.12 this would have introduced a refleak, as the branch never decrefs func. However, now Py_None is immortal and we no longer have to worry about refcounting for it. Otherwise, I'd have suggested putting Py_XDECREF(func) inside the conditional block (the X meaning that func may be NULL).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The NULL == func here is a "Yoda condition". People write that because C lets you write if (func = NULL), and it doesn't do what you want (it always sets func to NULL).

Thanks for explaining this! I wondered what the rationale was here for writing it like this!

Huh. I checked the API docs for Py_None before filing this PR, and they didn't mention this function — they recommended using ==: https://docs.python.org/3/c-api/none.html. I guess I'll leave this as it is for now, since as you say, there are a bunch of other places in this file that use == :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.13 bugs and security fixes type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve error message from os.fspath if __fspath__ is set to None

4 participants