Skip to content

Add some enum utilities #8171

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

Merged
merged 1 commit into from
Mar 18, 2021
Merged

Add some enum utilities #8171

merged 1 commit into from
Mar 18, 2021

Conversation

SpexGuy
Copy link
Contributor

@SpexGuy SpexGuy commented Mar 7, 2021

This PR adds some enum utilities, including EnumSet, EnumMap, and EnumArray data structures, and some helper functions that would have been useful in some recent enum-heavy work I have been doing. It conflicts with #7649 and #7882, which add similar functionality. However, this implementation is more complete and handles extern enums with multiple fields mapped to the same value.

I'm open to any suggestions for improvements or name changes.

Copy link
Contributor

@daurnimator daurnimator left a comment

Choose a reason for hiding this comment

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

It conflicts with #7649

Note that #7649 adds use-sites at places throughout the standard library. If this is merged instead, please consider adding them too

/// the total number of items which have no matching enum key (holes in the enum
/// numbering). So for example, if an enum has values 1, 2, 5, and 6, max_unused_slots
/// must be at least 3, to allow unused slots 0, 3, and 4.
fn directEnumArrayLen(comptime E: type, comptime max_unused_slots: comptime_int) comptime_int {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be offset by a min

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent of this api is to be directly indexed by the integer value of the enum, hence the direct part of the name. The max_unused_slots parameter ensures you don't have more padding than you intended. If you want a better packing, the EnumArray type will offset by a min.

if (unused_slots > max_unused_slots) {
const unused_str = std.fmt.comptimePrint("{d}", .{unused_slots});
const allowed_str = std.fmt.comptimePrint("{d}", .{max_unused_slots});
@compileError("Cannot create a direct enum array for "++@typeName(E)++". It would have "++unused_str++" unused slots, but only "++allowed_str++" are allowed.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably return an error rather than a @compileError, otherwise a user cannot provide a fallback as an enum grows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand the use case for catching this error. This is a safety check to make sure that you are not accidentally making an array with a large number of empty slots. If this triggers, the way to handle it is to inspect it and decide whether to increase the allowed number of unused slots or switch to a denser packing like EnumArray. If you are writing generic code that calls this function, your code should accept this parameter alongside the enum.

Can you provide a more specific example where you would want to catch this?

@daurnimator daurnimator added the standard library This issue involves writing Zig code for the standard library. label Mar 7, 2021
@SpexGuy
Copy link
Contributor Author

SpexGuy commented Mar 7, 2021

Is this CI failure related to my changes? I don't see an error message so I'm not sure what the problem is. Maybe the additional comptime execution for the tests causing OOM?

@g-w1
Copy link
Contributor

g-w1 commented Mar 7, 2021

You can close and re-open the pr to re-run the ci.

@SpexGuy SpexGuy closed this Mar 7, 2021
@SpexGuy SpexGuy reopened this Mar 7, 2021
@data-man
Copy link
Contributor

It conflicts with #7882

Absolutely no.

@ifreund
Copy link
Member

ifreund commented Mar 18, 2021

@SpexGuy should this close #793?

@andrewrk
Copy link
Member

andrewrk commented Mar 18, 2021

Is this CI failure related to my changes? I don't see an error message so I'm not sure what the problem is. Maybe the additional comptime execution for the tests causing OOM?

I think that's it, unfortunately. master branch right now is taking a whopping 8.2 GiB to run std lib tests. This branch bumps it up to 8.4, which is hardly the fault of this code, but it seems to be the straw that broke the camel's back.

I want to merge this though, so it seems I will have to find some other way to recover the extra memory.

@andrewrk
Copy link
Member

I'm hoping these are enough:

5cbb642
a6f5aa7
5e5b35f

If not, I'll do more!

@andrewrk andrewrk merged commit 96ae451 into ziglang:master Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants