Convert StructTypeField to a specific type. #4492
Conversation
jonmeow
force-pushed
the
struct-type-field
branch
from
ded19ba
to
405a4af
Compare
There was a problem hiding this comment.
Nice improvement!
I was expecting you would have to add handling for StructTypeFieldsId to check/deduce.cpp, but it looks like the corresponding test is a TODO, see
https://github.com/carbon-language/carbon-lang/blob/trunk/toolchain/check/testdata/function/generic/deduce.carbon#L84
I think this change makes fixing that test pretty straightforward. If you were feeling ambitious, you could fix it in this PR or a follow-up.
| // CHECK:STDERR: let w: i32 = {.def = 1, .def = 2}.def; | ||
| // CHECK:STDERR: ^~~ | ||
| // CHECK:STDERR: fail_duplicate_name.carbon:[[@LINE+4]]:16: note: field with the same name here [StructNamePrevious] | ||
| // CHECK:STDERR: ^ |
There was a problem hiding this comment.
This location information doesn't seem to be as good as before.
There was a problem hiding this comment.
Fixed, should be back to what it was. Probably a better approach for transferring names too.
Thanks for pointing this out, but this PR is already very big and complex, and I don't think fixing an unrelated TODO should be part of it. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Follow-on to #4492 . --------- Co-authored-by: Josh L <[email protected]>
This converts
StructTypeFieldfrom an instruction to a dedicated type, with its own store. This had originated from discussing how.GetAs<SemIR::StructTypeField>was more prevalent than for other instructions, but is probably more interesting for the storage savings (16 bytes StructTypeField + 4 byte LocId + 4 byte InstId -> 8 byte StructTypeField).Due to the different structure, these now have their own stack during construction, reducing (but not eliminating)
args_type_info_stack_use-cases.The test changes of different InstIds is expected because structs and classes generate fewer instructions now. Other than that, results should remain the same.
I'm generally trying to avoid unrelated cleanup here due to the PR size, though I did scrutinize the
VerifyOnFinishcalls, adding one and commenting others (putting them in member order because that's how I was checking what was verified and what wasn't).