Skip to content

Conversation

@skirpichev
Copy link
Contributor

@skirpichev skirpichev commented Dec 11, 2025

@encukou
Copy link
Member

encukou commented Dec 12, 2025

Did you consider using a PyStructSequence instead? It might be a better fit for C code.

@skirpichev
Copy link
Contributor Author

I'm not sure about issue itself, see #142595 (comment)

But as an excuse for refactoring this sounds much better. I'll rework the pr.

@skirpichev skirpichev marked this pull request as draft December 12, 2025 09:15
@skirpichev skirpichev marked this pull request as ready for review December 12, 2025 10:30
@skirpichev
Copy link
Contributor Author

Hmm, no. I don't think using PyStructSequence is a good idea.

We can't create this type just with PyStructSequence_NewType. The namedtype DecimalType supports three arguments, while new type - only one (a sequence). Probably, I can override tp_new, but at the end - things looks more complex with all this, not simpler.

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

In general, it might be good to add a way to create PyStructSequences that are more namedtuple-like. But, yeah, I agree it's too much work for this issue.

@encukou encukou merged commit be5e0dc into python:main Dec 12, 2025
56 checks passed
@encukou encukou added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Dec 12, 2025
@miss-islington-app
Copy link

Thanks @skirpichev for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @skirpichev for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 12, 2025
…itialization (pythonGH-142608)

(cherry picked from commit be5e0dc)

Co-authored-by: Sergey B Kirpichev <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 12, 2025
…itialization (pythonGH-142608)

(cherry picked from commit be5e0dc)

Co-authored-by: Sergey B Kirpichev <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Dec 12, 2025

GH-142622 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Dec 12, 2025
@bedevere-app
Copy link

bedevere-app bot commented Dec 12, 2025

GH-142623 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Dec 12, 2025
@skirpichev skirpichev deleted the fix-decimal-exec/142595 branch December 12, 2025 11:00
@skirpichev
Copy link
Contributor Author

In general, it might be good to add a way to create PyStructSequences that are more namedtuple-like. But, yeah, I agree it's too much work for this issue.

I don't think that the amount of work is the problem: it seems, that interfaces are different. The namedtype has additional class method _make() with signature of PyStructSequences constructor.

Perhaps, some PyStructSequence_NewType2(), that creates a type with the right constructor? If not, I think we should just document the difference. I was certainly misguided by sentence:

Struct sequence objects are the C equivalent of namedtuple() objects

@encukou
Copy link
Member

encukou commented Dec 12, 2025

Yeah, that should reference the glossary term, not collections.namedtuple specifically.
Doc fix here: #142626

@encukou
Copy link
Member

encukou commented Dec 12, 2025

Perhaps, some PyStructSequence_NewType2(), that creates a type with the right constructor?

Yeah, this, with a new versioned variant of PyStructSequence_Desc rather than hardcoding the different behaviour.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants