Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support initialization of specific classes from struct literals #4320

Merged
merged 4 commits into from
Sep 19, 2024

Conversation

zygoloid
Copy link
Contributor

Add support for initializing types like GenericClass(i32) from a struct literal. A new kind of instruction, complete_type_witness, is added to the class definition to track the object representation type so that it's visible to the generics machinery. Accesses to the object representation of a class have all been updated to pass in the class's SpecificId so that the types of the fields of the specific class are used instead of the types of the fields of the generic class in places that look at the object representation -- primarily class initialization.

@zygoloid
Copy link
Contributor Author

Most of the test changes are caused by the change in representation. For the functionality changes, see toolchain/check/testdata/class/generic/init.carbon.

Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Looks good, mainly a minor question if you want to start writing new diagnostics using #4321 since it seems we have consensus there.

const auto& class_info = context.classes().Get(class_id);
if (class_info.base_id.is_valid()) {
CARBON_DIAGNOSTIC(AdaptWithBase, Error,
"Adapter cannot have a base class.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Getting to the diagnostic phrasing discussion, should this be "Adapter has a base class"?

Note I don't have a strong opinion, just asking to try to get a similar voice as we add diagnostics.

Oh, but more seriously... you can probably remove the capital and period now. The related PR is still pending but I think it'd make sense to stop writing those into new diagnostics.

if (class_info.base_id.is_valid()) {
CARBON_DIAGNOSTIC(AdaptWithBase, Error,
"Adapter cannot have a base class.");
CARBON_DIAGNOSTIC(AdaptBaseHere, Note, "`base` declaration is here.");
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW with this and AdaptFieldsHere, had you considered "AdaptWithBaseHere", using the main error as a prefix to tie the note more closely together? (or, AdaptBase/AdaptFields, dropping "With")

@zygoloid zygoloid added this pull request to the merge queue Sep 19, 2024
@zygoloid
Copy link
Contributor Author

Per in-person discussion: the diagnostic stuff here was just moved to a different function, not added or renamed, so I'm going to leave those comments for a separate PR.

Merged via the queue into carbon-language:trunk with commit 2044366 Sep 19, 2024
8 checks passed
@zygoloid zygoloid deleted the toolchain-init-generic-class branch September 19, 2024 19:51
github-merge-queue bot pushed a commit that referenced this pull request Sep 20, 2024
Doing this to a couple of diagnostics was suggested in review comments
on #4320, so I've applied the suggestions across the whole file.

---------

Co-authored-by: Jon Ross-Perkins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants