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

Move Type.Data.ip_index and .either to Type #2186

Closed
wants to merge 4 commits into from

Conversation

FnControlOption
Copy link
Contributor

@FnControlOption FnControlOption commented Feb 12, 2025

The is_type_val field is redundant for ip_index and either, so let's turn Type into a tagged union with those and the original struct as the possible variants. Currently, I've named the new variant as dynamic, but I'm open to suggestions!

Before After
pub const Type = struct {
    data: Data,
    is_type_val: bool,

    pub const Data = union(enum) {
        // ...

        compile_error: NodeWithHandle,

        either: []const EitherEntry,

        ip_index: struct {
            node: ?NodeWithHandle = null,
            index: InternPool.Index,
        },

        pub const EitherEntry = struct {
            type_data: Data,
            descriptor: []const u8,
        };
    };

    // ...

    pub const TypeWithDescriptor = struct {
        type: Type,
        descriptor: []const u8,
    };

    // ...
};
pub const Type = union(enum) {
    dynamic: struct {
        data: Data,
        is_type_val: bool,
    },

    ip_index: struct {
        node: ?NodeWithHandle = null,
        index: InternPool.Index,
    },

    either: []const TypeWithDescriptor,

    pub const Data = union(enum) {
        // ...

        compile_error: NodeWithHandle,
    };

    // ...

    pub const TypeWithDescriptor = struct {
        type: Type,
        descriptor: []const u8,
    };

    // ...
};

@Techatrix
Copy link
Member

What exactly is this PR trying to improve? The PR description implies code quality by removing a redundant field but the code now requires analysis to switch twice every time it want to extract some information out of Type. Having is_type_val is not a big issue for ip_index and either since it avoids computing it (which happens quite often) and the rest of the code remains more readable. Hypothetically, if ZLS was already in the "After" state, I would accept a PR that would change it to the "Before" state.

This change will also conflicts with #2180 since it tries to add a new field to Type.

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