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-45459: Rename buffer.h to pybuffer.h by vstinner · Pull Request #31201 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Feb 7, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Rename Include/buffer.h header file to Include/pybuffer.h to avoid
conflits with projects having an existing buffer.h header file.

  • Incude pybuffer.h before object.h in Python.h.
  • Remove #include "buffer.h" from Include/cpython/object.h.
  • Add a forward declaration of the PyObject type in pybuffer.h to fix
    an inter-dependency issue.

https://bugs.python.org/issue45459

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rdb and pmp-p reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@vstinner
bpo-45459: Rename buffer.h to pybuffer.h
75719a6 
Rename Include/buffer.h header file to Include/pybuffer.h to avoid
conflits with projects having an existing buffer.h header file.

* Incude pybuffer.h before object.h in Python.h.
* Remove #include "buffer.h" from Include/cpython/object.h.
* Add a forward declaration of the PyObject type in pybuffer.h to fix
  an inter-dependency issue.
@vstinner
vstinner requested a review from a team as a code owner February 7, 2022 17:48
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 7, 2022
@bedevere-bot bedevere-bot added the awaiting core review label Feb 7, 2022
@vstinner

vstinner commented Feb 7, 2022

Copy link
Copy Markdown
Member Author

cc @rdb @pmp-p @tiran
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tiran
tiran suggested changes Feb 7, 2022
View reviewed changes
Comment thread Include/pybuffer.h
*/

// Forward declaration
typedef struct _object PyObject;

@tiran tiran Feb 7, 2022

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.

That's ugly. I had a forward declaration in my first PR. @encukou asked me to remove it.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner Feb 7, 2022

Copy link
Copy Markdown
Member Author

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.

How is it "ugly"? Could you be more specific?

There are many similar code used in Python header files to fix inter-dependency issues. See for example Include/pystate.h.

Sorry, something went wrong.

Uh oh!

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

@encukou encukou Feb 7, 2022

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.

I didn't realize the reason it was there. It's ugly, but it might be a good way to solve the issue.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner Feb 7, 2022

Copy link
Copy Markdown
Member Author

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.

I elaborated the comment to explain why this "Forward declaration" is needed. Is it better?

Sorry, something went wrong.

Uh oh!

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

@zooba zooba Feb 7, 2022

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.

Why not forward-define Py_buffer in object.h instead? That's a smaller change anyway, and removes the ordering requirement completely, rather than simply forcing it to happen in a certain way.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner Feb 7, 2022

Copy link
Copy Markdown
Member Author

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.

I see Py_buffer as a C structure which is unrelated to Python objects. It's like a low-level thing "under" PyObject. In practice, it contains a PyObject* pointer and pybuffer.h contains other function definitions which use PyObject*.

I don't think that the order matters much, both orders work.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner Feb 22, 2022

Copy link
Copy Markdown
Member Author

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.

Why not forward-define Py_buffer in object.h instead?

It's not possible: the Py_buffer struct has no name on purpose, see @tiran's comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Feb 7, 2022

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting core review labels Feb 7, 2022
@vstinner
Elaborate the comment
7e32c1c
@vstinner

vstinner commented Feb 8, 2022

Copy link
Copy Markdown
Member Author

@tiran: Do you prefer to have a forward declaration of Py_buffer in object.h, or a forward declaration of PyObject in pybuffer.h? Well, I wrote my opinion on the order in a comment above, but I don't really care.

Or if you prefer, I can put all of these definitions in a new single header file and make sure that it's one of the first included header. As I said, there are other header files using such forward declarations to break cycles of dependencies between two header files.

In C, it's ok to use struct MyStruct before MyStruct is defined. What is not ok is to use MyType before MyType is defined. typedef struct MyStruct MyType; statement must come first.
All reactions

Sorry, something went wrong.

Uh oh!

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

tiran commented Feb 8, 2022

Copy link
Copy Markdown
Member

@encukou asked me to make Py_buffer an anon struct typedef. C does not support forward declaration of anon struct. You either have to give the Py_buffer struct a name again or keep your approach.
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Feb 8, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

You either have to give the Py_buffer struct a name again or keep your approach.

Well, this PR works and doesn't require to give a name to the anonymous Py_buffer struct :-)

@tiran: GitHub still tells me that there is "1 change requested" from your side. Can you either remove your -1 vote or vote +1 (approve)?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner merged commit 66b3cd7 into python:main Feb 22, 2022
@bedevere-bot bedevere-bot removed the awaiting changes label Feb 22, 2022
@bedevere-bot

bedevere-bot commented Feb 22, 2022

Copy link
Copy Markdown

@vstinner: Please replace # with GH- in the commit message next time. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner deleted the pybuffer_h branch February 22, 2022 22:11
@vstinner

vstinner commented Feb 22, 2022

Copy link
Copy Markdown
Member Author

I merged my fix. Thanks for reviews.

It's unclear to me why the Py_buffer struct has no name, but well, I'm fine with no giving it a name :-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@geofft geofft mentioned this pull request Jul 18, 2025
<lock.h> filename is too generic #136759
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou left review comments
@zooba zooba zooba left review comments
+1 more reviewer
@tiran tiran tiran requested changes
Reviewers whose approvals may not affect merge requirements

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.

6 participants

@vstinner @bedevere-bot @tiran @encukou @zooba @the-knights-who-say-ni
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.