Skip to content

gh-146615: Fix format specifiers in Objects/ directory#146620

Draft
sunmy2019 wants to merge 1 commit intopython:mainfrom
sunmy2019:gh-146615-4
Draft

gh-146615: Fix format specifiers in Objects/ directory#146620
sunmy2019 wants to merge 1 commit intopython:mainfrom
sunmy2019:gh-146615-4

Conversation

@sunmy2019
Copy link
Copy Markdown
Member

@sunmy2019 sunmy2019 commented Mar 30, 2026

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

PyErr_Format(PyExc_TypeError,
"descriptor '%V' needs a type, not '%s', as arg 2",
descr_name((PyDescrObject *)descr),
descr_name((PyDescrObject *)descr), "?",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm sorry but what is this ??

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Mar 30, 2026

Choose a reason for hiding this comment

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

%V takes two arguments: PyObject* (Unicode object) and const char*. The latter is used when the former is NULL. There was a hard bug here, this code did not work and perhaps crashed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh! I didn't know about the %V format (I think I never used it...)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is worth to extract this case into a separate PR, and add test for it. Unlike to other cases, it never worked on any platforms.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, a separate test would be fine I guess. And it could have its proper NEWS entry if necessary.

@picnixz
Copy link
Copy Markdown
Member

picnixz commented Mar 30, 2026

I would suggest that we merge the various PRs without a NEWS entry and that we create one new entry for many PRs and add a commit message with links to those PRs.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants