lazy field index #4514
lazy field index #4514
Conversation
dwblaikie
force-pushed
the
lazy_field_index
branch
from
7bda9ba
to
9ce4e22
Compare
|
Ended up deciding maybe we could use this review as the central point for the design discussion - since the other two patches are a bit more drafty, so I'll leave them in draft mode, but linked from here for context - but maybe keeping the design discussion here in this PR. |
This way if the type_id of the FieldDecl is anything other than Error or UnboundElementType, this should check-fail at the `GetAs<UnboundElementType>`. We could add a more explicit check, but I tend to err towards relying on implicit checks to avoid making the code too noisy
toolchain/check/handle_class.cpp
Outdated
| llvm::SmallVector<SemIR::StructTypeField> struct_type_fields; | ||
| struct_type_fields.reserve(context.field_decls_stack().PeekArray().size()); | ||
|
|
||
| return CheckCompleteAdapterClassType(context, node_id, class_id, fields_id); | ||
| return CheckCompleteAdapterClassType( | ||
| context, node_id, class_id, | ||
| AddStructTypeFields(context, struct_type_fields)); |
There was a problem hiding this comment.
We're going to a bit of effort to build a collection of fields for an adapter here (and adapters can't have fields). I wonder if we could sink this field handling code down into CheckCompleteAdapterClassType and have it just look at and diagnose the first element in the top of field_decls_stack if it's non-empty (and in any case pop it).
There was a problem hiding this comment.
Oh, sure - b53b6b1 - it has a minor observable sem-ir output change, which is that the field decls for the adapter have invalid indexes, since they never get updated/set by AddstructTypeFields - since the fields are invalid anyway, that seems fine?
toolchain/check/handle_class.cpp
Outdated
| auto field_decl = context.insts().GetAs<SemIR::FieldDecl>(field_decl_id); | ||
| field_decl.index = | ||
| SemIR::ElementIndex{static_cast<int>(struct_type_fields.size())}; | ||
| context.sem_ir().insts().Set(field_decl_id, field_decl); |
There was a problem hiding this comment.
Rather than directly setting the instruction, I think it'd be good to add something like auto Check::Context::ReplaceInstPreservingConstantValue(SemIR::InstId inst_id, SemIR::Inst inst) -> void, analogous to the existing ReplaceInstBeforeConstantUse. For extra safety, maybe we could make it recompute the constant value after the replacement and CHECK-fail if it's different.
There was a problem hiding this comment.
Sure, done in 89b88ff - at least a best guess at the requested assert/figuring out how the constant evaluation works. (since the constant value of a FieldDecl is currently just the inst_id itself, so it can't change, so I couldn't really think of anything to do to test the assertion works (like I tried modifying the FieldDecl's name_id or type_id, but those aren't part of the constant))
Co-authored-by: Richard Smith <[email protected]>
Co-authored-by: Richard Smith <[email protected]>
Co-authored-by: Richard Smith <[email protected]>
Co-authored-by: Richard Smith <[email protected]>
We considered a couple of other options for this:
ElementIndexnumbering vptr-ignorant, and do +1 offsets as needed - seems subtle/easy to missBut currently moving forward with this direction - of initializing field indexes with an invalid value until the end of the class definition, then assigning field indexes during construction of the class's object representation struct type. This direction might reinforce/help avoid premature access to the object representation before the class is complete, and give a single place where class layout is done (at class completion) if we want to add more options there, such as class layout optimizations, etc.
This patch still has problems with object initialization (that #4515 does not have/does address) but does address normal
obj.memberaccess correctly.