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

Improve std.StaticStringMap.initComptime #19936

Closed
clickingbuttons opened this issue May 10, 2024 · 9 comments
Closed

Improve std.StaticStringMap.initComptime #19936

clickingbuttons opened this issue May 10, 2024 · 9 comments

Comments

@clickingbuttons
Copy link
Contributor

clickingbuttons commented May 10, 2024

Status quo uses an anonymous struct of struct{ []const u8, T }:

const NameMap = std.StaticStringMap(@This()).initComptime(.{
    .{ "c89", .c89 },
    .{ "c90", .c89 },
    .{ "iso9899:1990", .c89 },
});

I propose a more map-like anonymous struct:

const NameMap = std.StaticStringMap(@This()).initComptime(.{
    .c89 = .c89,
    .c90 = .c89,
    .@"iso9899:1990" = .c89,
});

I don't think this is a design issue as long as Zig identifiers can be any arbitrary bytes.

Edit: This would also prevent accidentally initializing the same key twice at compile time.

const NameMap = std.StaticStringMap(@This()).initComptime(.{
    .{ "c89", .c89 },
    .{ "c89", .c90 },
});
@leecannon
Copy link
Contributor

leecannon commented May 10, 2024

This change would make building the key value list pragmatically much more complicated, you would need to use @Type.

@Pyrolistical
Copy link
Contributor

The reason why initComptime takes in a slice of tuples is so you construct the list at comptime:

const Enum = ErrorEnum(ErrorSet);
const error_to_enum = error_to_enum: {
    const ErrorEnumItem = std.meta.Tuple(&[_]type{ []const u8, Enum });
    comptime var error_to_enum_list: [es.len]ErrorEnumItem = undefined;
    inline for (0.., es) |i, err| {
        error_to_enum_list[i] = .{
            err.name,
            @field(Enum, err.name),
        };
    }

    break :error_to_enum std.StaticStringMap(Enum).initComptime(&error_to_enum_list);
};

@clickingbuttons
Copy link
Contributor Author

My real issue with the current API is that nothing stops you from passing multiple tuples with the same key. What do you think the following code prints?

pub fn main() !void {
    const Foo = enum { a, b };
    const NameMap = std.StaticStringMap(Foo).initComptime(.{
        .{ "c89", .b },
        .{ "c89", .a },
    });

    std.debug.print("{}\n", .{ NameMap.get("c89").? });
}

The answer may surprise you. This kind of mistake is a common copy-paste error.

This change would make building the key value list pragmatically much more complicated, you would need to use @Type.

No usage in the stdlib builds a key value list at comptime. Every usage is truly a static string map. I consider @Pyrolistical's use case an exception rather than the norm.

If you really want convenient support for building a list from a comptime var, there could be a initComptimeList that builds a @Type and correctly errors on duplicate keys.

@nektro
Copy link
Contributor

nektro commented May 13, 2024

the compiler not doing it, doesn't mean its not done out in the wild. I would expect the example above to compile error (through the use of a comptime assertion)

@Pyrolistical
Copy link
Contributor

@silversquirl
Copy link
Contributor

No usage in the stdlib builds a key value list at comptime.

That's just not true

zig/lib/std/meta.zig

Lines 25 to 33 in 6a65561

const kvs = comptime build_kvs: {
const EnumKV = struct { []const u8, T };
var kvs_array: [@typeInfo(T).Enum.fields.len]EnumKV = undefined;
for (@typeInfo(T).Enum.fields, 0..) |enumField, i| {
kvs_array[i] = .{ enumField.name, @field(T, enumField.name) };
}
break :build_kvs kvs_array[0..];
};
const map = std.StaticStringMap(T).initComptime(kvs);

@clickingbuttons
Copy link
Contributor Author

You're correct @silversquirl, I missed that one! My point still stands that the majority of usage, and I'd argue the intended usage is passing a static map-like structure rather than a built list.

@paperdave
Copy link

I don't think this is a design issue as long as Zig identifiers can be any arbitrary bytes.

there are two current limits on identifiers that i am aware of:

initComptime(.{
    .@"" = 1,     // identifier cannot be empty
    .@"\x00" = 2, // identifier cannot contain null bytes
});

@clickingbuttons
Copy link
Contributor Author

I think @paperdave 's example is enough of a reason not make a map API the default.

I'll open a separate issue to make duplicate keys a comptime error.

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

No branches or pull requests

6 participants