Skip to content
This repository was archived by the owner on Aug 21, 2025. It is now read-only.

Conversation

@ngoldbaum
Copy link

Followup for #5 to more fully disentangle CT_IS_OPAQUE from CT_UNDER_CONSTRUCTION.

@ngoldbaum ngoldbaum mentioned this pull request May 29, 2025
6 tasks
@ngoldbaum
Copy link
Author

Looks like CI passes - this is ready for review.

@ngoldbaum ngoldbaum requested a review from colesbury May 30, 2025 14:59

assert(!((ct->ct_flags & CT_IS_OPAQUE) ||
(ct->ct_flags_mut & CT_UNDER_CONSTRUCTION)));
// should have been unset during init for the ctype
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand this comment. I'd think that the assert below is because opaque structs never have lazy field lists.

Copy link
Author

Choose a reason for hiding this comment

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

I'd think that the assert below is because opaque structs never have lazy field lists.

This is what I was going for. I'll edit the comment.

/* This is called by force_lazy_struct() in _cffi_backend.c */
assert(ct->ct_flags & (CT_STRUCT | CT_UNION));

assert(!(ct->ct_flags_mut & CT_UNDER_CONSTRUCTION));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like it might be a temporary assert. I think we can end up with concurrent calls to do_realize_lazy_struct() on the same CTypeDescrObject

Copy link
Author

Choose a reason for hiding this comment

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

I think @kumaraditya303 plans to turn the CT_UNDER_CONSTRUCTION flag into a uint8 member of the CTypeDescrObject struct to facilitate reading and writing to it using atomic operations implemented using something like npy_atomic.h, which is a bare-bones version of pyatomic.h.

When that refactor is ready this assert can be moved or deleted when we're sure concurrent calls to b_complete_struct_or_union are safe.

@ngoldbaum ngoldbaum merged commit 3ad732d into Quansight-Labs:main Jun 3, 2025
54 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants