Add CT_UNDER_CONSTRUCTION to indicate that a type is being mutated #5
Conversation
|
Looks like I need to test on Linux.... |
src/c/realize_c_type.c
Outdated
| int is_opaque = (s->flags & _CFFI_F_OPAQUE); | ||
| flags |= is_opaque ? CT_IS_OPAQUE : 0; |
There was a problem hiding this comment.
Where is this from? It doesn't look to me like it has any effect as new_struct_or_union_type below always sets CT_IS_OPAQUE:
There was a problem hiding this comment.
I changed the line you linked to in this PR to unconditionally set CT_UNDER_CONSTRUCTION. Let me see if I make the change you suggested in your other comment if I can get rid of this.
src/c/realize_c_type.c
Outdated
| ct->ct_flags &= ~CT_IS_OPAQUE; | ||
| ct->ct_length = s->alignment; /* may be -1 */ | ||
| ct->ct_flags_mut |= CT_LAZY_FIELD_LIST; | ||
| ct->ct_flags_mut &= ~CT_UNDER_CONSTRUCTION; |
There was a problem hiding this comment.
I think this should remain as it was before:
-
We should do
ct->ct_flags &= ~CT_IS_OPAQUEbecausenew_struct_or_union_typeunconditionally setCT_IS_OPAQUEand this isn't an opaque struct ( because of!(s->flags & _CFFI_F_OPAQUE)) -
No need to do
ct->ct_flags_mut &= ~CT_UNDER_CONSTRUCTIONbecausect_flags_mutwas initialized to zero.
Even though we want ct_flags to be immutable, it's okay to clear CT_IS_OPAQUE here because we are effectively doing it as part of the struct construction; the construction is just kind of awkwardly split between new_struct_or_union_type and this function.
| return ((ct->ct_flags & CT_IS_OPAQUE) || | ||
| (ct->ct_flags_mut & CT_UNDER_CONSTRUCTION)); |
There was a problem hiding this comment.
I'm a little skeptical of this: it feels like we haven't fully disentangled CT_UNDER_CONSTRUCTION from CT_IS_OPAQUE, but I think it's fine for now.
There was a problem hiding this comment.
I think we should entangle them fully, otherwise it will be hard to make it thread safe
ab9b3df to
1d70aa7
Compare
|
I'd like to work on moving the other two mutable flags to |
Also splits out
CT_LAZY_FIELD_LISTinto a new listing of mutable flags, marking the old value as unused.c.f. #2 (comment).
The next cleanup after this is to do the same thing for
CT_CUSTOM_FIELD_POSandCT_WITH_PACKED_CHANGE.