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

Replace ct_is_hidden with asserts #7

Merged
merged 4 commits into from
Jun 3, 2025
Merged

Replace ct_is_hidden with asserts #7

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e33ecc5
rearrange and add asserts in do_realize_lazy_struct
ngoldbaum May 29, 2025
bc7f192
remove unhelpful comment
ngoldbaum May 29, 2025
2344819
replace ct_is_hidden with asserts
ngoldbaum May 29, 2025
a93b351
clarify comment
ngoldbaum Jun 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 6 additions & 12 deletions src/c/_cffi_backend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,16 +630,10 @@ get_field_name(CTypeDescrObject *ct, CFieldObject *cf); /* forward */
static int do_realize_lazy_struct(CTypeDescrObject *ct);
/* forward, implemented in realize_c_type.c */

static int ct_is_hidden(CTypeDescrObject *ct)
{
return ((ct->ct_flags & CT_IS_OPAQUE) ||
(ct->ct_flags_mut & CT_UNDER_CONSTRUCTION));
}

static PyObject *ctypeget_fields(CTypeDescrObject *ct, void *context)
{
if (ct->ct_flags & (CT_STRUCT | CT_UNION)) {
if (!ct_is_hidden(ct)) {
if (!(ct->ct_flags & CT_IS_OPAQUE)) {
CFieldObject *cf;
PyObject *res;
if (force_lazy_struct(ct) < 0)
Expand Down Expand Up @@ -1950,7 +1944,7 @@ get_alignment(CTypeDescrObject *ct)
int align;
retry:
if ((ct->ct_flags & (CT_PRIMITIVE_ANY|CT_STRUCT|CT_UNION)) &&
!ct_is_hidden(ct)) {
!(ct->ct_flags & CT_IS_OPAQUE)) {
align = ct->ct_length;
if (align == -1 && (ct->ct_flags_mut & CT_LAZY_FIELD_LIST)) {
force_lazy_struct(ct);
Expand Down Expand Up @@ -3347,9 +3341,8 @@ static PyObject *cdata_dir(PyObject *cd, PyObject *noarg)
if (ct->ct_flags & CT_POINTER) {
ct = ct->ct_itemdescr;
}
// NJG: not sure this does the right thing if a type is already under construction
if ((ct->ct_flags & (CT_STRUCT | CT_UNION)) &&
!ct_is_hidden(ct)) {
!(ct->ct_flags & CT_IS_OPAQUE)) {

/* for non-opaque structs or unions */
if (force_lazy_struct(ct) < 0)
Expand Down Expand Up @@ -5319,11 +5312,12 @@ static PyObject *b_complete_struct_or_union(PyObject *self, PyObject *args)

is_union = ct->ct_flags & CT_UNION;
if (!((ct->ct_flags & CT_UNION) || (ct->ct_flags & CT_STRUCT)) ||
!ct_is_hidden(ct)) {
!((ct->ct_flags & CT_IS_OPAQUE) || (ct->ct_flags_mut & CT_UNDER_CONSTRUCTION))) {
PyErr_SetString(PyExc_TypeError,
"first arg must be a non-initialized struct or union ctype");
return NULL;
}
assert((ct->ct_flags & CT_IS_OPAQUE) ^ (ct->ct_flags_mut & CT_UNDER_CONSTRUCTION));
ct->ct_flags_mut &= ~(CT_CUSTOM_FIELD_POS | CT_WITH_PACKED_CHANGE);

alignment = 1;
Expand Down Expand Up @@ -5352,7 +5346,7 @@ static PyObject *b_complete_struct_or_union(PyObject *self, PyObject *args)
goto error;

if ((ftype->ct_flags & (CT_STRUCT | CT_UNION)) &&
!ct_is_hidden(ftype)) {
!(ftype->ct_flags & CT_IS_OPAQUE)) {
/* force now the type of the nested field */
if (force_lazy_struct(ftype) < 0)
return NULL;
Expand Down
8 changes: 4 additions & 4 deletions src/c/realize_c_type.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,7 +778,7 @@ static int do_realize_lazy_struct(CTypeDescrObject *ct)
{
/* 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

@colesbury colesbury Jun 2, 2025

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

@ngoldbaum ngoldbaum Jun 3, 2025

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.

if (ct->ct_flags_mut & CT_LAZY_FIELD_LIST) {
builder_c_t *builder;
char *p;
Expand All @@ -787,8 +787,8 @@ static int do_realize_lazy_struct(CTypeDescrObject *ct)
const struct _cffi_field_s *fld;
PyObject *fields, *args, *res;

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

@colesbury colesbury Jun 2, 2025

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

@ngoldbaum ngoldbaum Jun 3, 2025

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.

assert(!(ct->ct_flags & CT_IS_OPAQUE));

builder = ct->ct_extra;
assert(builder != NULL);
Expand Down Expand Up @@ -884,11 +884,11 @@ static int do_realize_lazy_struct(CTypeDescrObject *ct)

assert(ct->ct_stuff != NULL);
ct->ct_flags_mut &= ~CT_LAZY_FIELD_LIST;
assert(!(ct->ct_flags_mut & CT_UNDER_CONSTRUCTION));
Py_DECREF(res);
return 1;
}
else {
assert(!(ct->ct_flags_mut & CT_UNDER_CONSTRUCTION));
return 0;
}
}
Loading