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

Signed types for enum tags #2459

Merged
merged 5 commits into from
May 12, 2019
Merged

Signed types for enum tags #2459

merged 5 commits into from
May 12, 2019

Conversation

LemonBoy
Copy link
Contributor

@LemonBoy LemonBoy commented May 9, 2019

Implements #641

The first commit makes sure we produce a valid enum from the C ABI point of view, gcc has a extension where the value can be promoted to long but I decided to stick to ANSI C.

The too small to hold all bits check is gone, replaced by some simple overflow detection mechanism that's now correct for signed types too.

The enumerator value allocation is still starting from 0, no change on that front.

What I found extremely strange is that the value allocation doesn't follow the C semantics where the previous specified value is used as starting point. I can provide a patch to implement this semantic if you wish.

@andrewrk
Copy link
Member

What I found extremely strange is that the value allocation doesn't follow the C semantics where the previous specified value is used as starting point. I can provide a patch to implement this semantic if you wish.

Certainly at least for extern enums, they should follow C semantics of value numbering. Other enums - I don't have an answer right now. The behavior you're observing is probably a mistake (or simply was never implemented)

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Reworked logic looks good.

What do you think about my comment regarding the allowed ABI types for extern?

src/analyze.cpp Outdated Show resolved Hide resolved
src/analyze.cpp Outdated Show resolved Hide resolved
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

After some minor compile error fixes I think this is good to go

}
bigint_init_bigint(&type_enum_field->value, &proposed_value);
}
ErrorMsg *msg = add_node_error(g, field_node,
Copy link
Member

Choose a reason for hiding this comment

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

If you make this field_node->data.struct_field.value then the compile error test won't need updated, right? It seems better to point at the value than the field. Likewise with entry->value->data.struct_field.value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

field_node->data.struct_field.value is null if the user didn't specify it, I've made it point to the field declaration because that's always present.

I can make it choose either of them based on whether value is null or not but I'm not very fond of this "asymmetry" in the error messages.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, that makes sense!

src/analyze.cpp Outdated Show resolved Hide resolved
src/analyze.cpp Outdated Show resolved Hide resolved
@andrewrk andrewrk merged commit edcc7c7 into ziglang:master May 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants