GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-30459: Cast the result of PyList_SET_ITEM() to void by ZackerySpytz · Pull Request #19975 · python/cpython

Conversation

@ZackerySpytz

@ZackerySpytz ZackerySpytz commented May 7, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Do the same for PyTuple_SET_ITEM().

https://bugs.python.org/issue30459

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ZackerySpytz
bpo-30459: Cast the result of PyList_SET_ITEM() to void
febb5a6 
Do the same for PyTuple_SET_ITEM().
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 7, 2020
@bedevere-bot bedevere-bot added the awaiting review label May 7, 2020
@corona10
corona10 requested a review from vstinner May 7, 2020 06:04
vstinner
vstinner reviewed May 7, 2020
View reviewed changes
Comment thread Include/cpython/listobject.h

#define PyList_GET_ITEM(op, i) (_PyList_CAST(op)->ob_item[i])
#define PyList_SET_ITEM(op, i, v) (_PyList_CAST(op)->ob_item[i] = (v))
#define PyList_SET_ITEM(op, i, v) ((void)(_PyList_CAST(op)->ob_item[i] = (v)))

@vstinner vstinner May 7, 2020

Copy link
Copy Markdown
Member

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.

If a change is made, I would prefer to convert this macro to a static inline function. You need to keep a macro for _PyList_CAST(). For example, declare the static inline function with name _PyList_SET_ITEM(). See Py_INCREF() for another example. Also, while we are here, I would prefer to use better name than "i" and "v". For example: PyList_SET_ITEM(list, index, value).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed May 7, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

The initial issue seems to show that PyList_SET_ITEM() result can be used, but also that it is used in practice:
https://bugs.python.org/issue30459#msg294362

So I would suggest to document in the C API changes, Porting to Python 3.9:
https://docs.python.org/dev/whatsnew/3.9.html#id2

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner

vstinner commented May 7, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

By the way, I added Py_SET_REFCNT(), Py_SET_TYPE() and Py_SET_SIZE() in Python 3.9, but I left the related Py_REFCNT(), Py_TYPE() and Py_SIZE() unchanged. It remains possible to use them as l-value for example: Py_REFCNT(ob) = 1;.

I expect that few C extensions use Py_REFCNT(ob) = 1;, but maybe a few use Py_TYPE(op) = type;. For Py_SIZE(ob) = size;, Python 3.8 didn't provide any "abstract" setter function/macro, direct access to the member is also possible: ((PyVarObject*)ob)->ob_size = size;,

I chose to add new macros in Python 3.9, and maybe change the macros in Python 3.10. The change is part of a larger project:
https://github.com/vstinner/misc/blob/master/cpython/pep-opaque-c-api.rst

See bpo-39573.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Dec 4, 2020

Copy link
Copy Markdown
Member

No activity since May, I close the PR. I created PR #23645 which converts PyTuple_SET_ITEM() and PyList_SET_ITEM() to static inline functions.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner closed this Dec 4, 2020
@serhiy-storchaka serhiy-storchaka reopened this Dec 5, 2020
serhiy-storchaka
serhiy-storchaka approved these changes Dec 5, 2020
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Dec 5, 2020
@vstinner
vstinner merged commit 556d97f into python:master Dec 5, 2020
@bedevere-bot bedevere-bot removed the awaiting merge label Dec 5, 2020
@vstinner vstinner mentioned this pull request Dec 5, 2020
bpo-30459: Convert PyTuple_SET_ITEM() to a function #23645
Closed
@vstinner

vstinner commented Dec 5, 2020

Copy link
Copy Markdown
Member

Thanks @ZackerySpytz, I merged your PR. I wrote PR #23654 to modify also PyCell_SET().
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@ZackerySpytz @adorilson
bpo-30459: Cast the result of PyList_SET_ITEM() to void (pythonGH-19975)
57a734e 
Do the same for PyTuple_SET_ITEM().
This was referenced Jul 5, 2022
[BUG] listobject.h:33:36: error: void value not ignored as it ought to be cython/cython#4873
Closed
listobject.h:33:36: error: void value not ignored as it ought to be #94507
Closed
@vstinner vstinner mentioned this pull request Mar 12, 2024
[C API] Avoid accessing PyObject and PyVarObject members directly: add Py_SET_TYPE() and Py_IS_TYPE(), disallow Py_TYPE(obj)=type #83754
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@ZackerySpytz @vstinner @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.