ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
34.6k stars 2.53k forks source link

Proposal: Rename packed struct to something more representative #12852

Open ikskuh opened 2 years ago

ikskuh commented 2 years ago

As Zig is a lot about communication, we should probably rename packed struct into something that represents more what it actually is.

Right now, a lot of people assume that packed struct follows similar rules as regular or extern structs, which isn't true anymore with stage2. Thus, a lot of confusion is created and people misuse packed structs accidently or try to solve problems with them where extern struct is the right hammer.

I propose to change the name of packed struct into something that clearly represents what it is. My current best guess would be bitrecord or bitfield, and respectivly rename packed union to bitunion (which is not a good name).

This change is also a good idea as you cannot put arrays or pointers into packed structs

dee0xeed commented 2 years ago

This change is also a good idea as you cannot put arrays or pointers into packed structs

I would say there is no need at all to hold pointers in packed structs - for me they are just very convenient feature when dealing with various binary network protocols or binary file formats.

ghost commented 2 years ago

I think bitfield would have been ideal, if it weren't for the analogous bitunion, which is quite terrible. Maybe the same clarification value can be achieved by renaming packed to bitpacked? Thinking further, extern could then become bytepacked.

leecannon commented 2 years ago

@zzyxyzz extern are not byte packed, you have to add explicit align(1) to non-byte aligned fields to get that behaviour

const HasPadding = extern struct {
    b: bool,
    n: u32,

    comptime {
        std.debug.assert(@sizeOf(HasPadding) == 8);
    }
};

const NoPadding = extern struct {
    b: bool,
    n: u32 align(1),

    comptime {
        std.debug.assert(@sizeOf(NoPadding) == 5);
    }
};
ghost commented 2 years ago

@leecannon, thanks for the correction. In that case, bytepacked could become an independent qualifier rather than replacing extern.

eLeCtrOssSnake commented 1 year ago

tl;dr; I think both names show exact purpose and are fine. I am against changing them.

extern struct - struct for usage in external modules/code/functions thus "extern". packed struct means bitpacked, which means it bitpacks integers. I think both names nail their task 100%. Andre Weissflog said on zig discord that he mistook packed struct with attribute((packed)) in C. But if we follow the zig philosophy then we shouldn't continue to carry this legacy into zig. Because we already have align(1). Packed means a different thing in zig and it's fine.

ikskuh commented 1 year ago

Andre Weissflog said on zig discord that he mistook packed struct with __attribute__((packed)) in C.

That's exactly what this is about. packed in C means byte packed. packed in Zig means bitpacked. This is an arbitrary choice and both are 100% legal choices. packed in Zig does not convey the information that it is bitpacked, and it's afaik the only language doing this right now. All other languages use bitfield or similar for this kind of construct.

extern in Zig also does not mean "external module/codes/functions", but "well defined memory layout based on the natural alignment of types". Using extern here isn't perfect, but doesn't spark as much confusion as packed. Even people using Zig for longer times confuse this all the time, which means we have a readability defect in the language.

eLeCtrOssSnake commented 1 year ago

Andre Weissflog said on zig discord that he mistook packed struct with __attribute__((packed)) in C.

That's exactly what this is about. packed in C means byte packed. packed in Zig means bitpacked. This is an arbitrary choice and both are 100% legal choices. packed in Zig does not convey the information that it is bitpacked, and it's afaik the only language doing this right now. All other languages use bitfield or similar for this kind of construct.

extern in Zig also does not mean "external module/codes/functions", but "well defined memory layout based on the natural alignment of types". Using extern here isn't perfect, but doesn't spark as much confusion as packed. Even people using Zig for longer times confuse this all the time, which means we have a readability defect in the language.

I don't understand the difference between extern and packed being a different level of readability defect because extern serves as much of a different purpose in Zig compared to C as packed.

eLeCtrOssSnake commented 1 year ago

The whole argument is that somebody confuses packed with C packed and now suddenly we have to change it. But we're not C. We're a different language.

ghost commented 1 year ago

The whole argument is that somebody confuses packed with C packed and now suddenly we have to change it. But we're not C. We're a different language.

I would argue that this issue goes deeper than naming and superficial differences between languages. I feel that bitpacking, while certainly a useful feauture, is the wrong default for dense, fixed-layout structures due to their limited composability: you cannot embed arrays or other non-packed structs, and you can have neither field pointers nor pointer fields. Byte-packed structs have none of these problems and should be the default packed layout unless you plan to do actual bit slicing.

However, since the packed struct semantics was changed, Zig no longer has a linear byte-packed struct. Yes, you can use extern with align(1), but besides being a bit of a pain, it is an abuse of a type intended for something else. From the documentation:

An extern struct has in-memory layout guaranteed to match the C ABI for the target. This kind of struct should only be used for compatibility with the C ABI.

extern struct does not actually guarantee any fixed layout, and even if it did, you'd have to know some details about the C ABI to be sure.

The expectation that packed struct should behave like _attribute__((packed)) is not just an accident of history. It actually makes sense. Hence I'd be very much in favor of reintroducing byte-packed struct semantics alongside bitfield, possibly with a syntax like packed(.bit), packed(.byte), packed(.C) as suggested by @tleydxdy in the other thread.

eLeCtrOssSnake commented 1 year ago

The whole argument is that somebody confuses packed with C packed and now suddenly we have to change it. But we're not C. We're a different language.

I would argue that this issue goes deeper than naming and superficial differences between languages. I feel that bitpacking, while certainly a useful feauture, is the wrong default for dense, fixed-layout structures due to their limited composability: you cannot embed arrays or other non-packed structs, and you can have neither field pointers nor pointer fields. Byte-packed structs have none of these problems and should be the default packed layout unless you plan to do actual bit slicing.

However, since the packed struct semantics was changed, Zig no longer has a linear byte-packed struct. Yes, you can use extern with align(1), but besides being a bit of a pain, it is an abuse of a type intended for something else. From the documentation:

An extern struct has in-memory layout guaranteed to match the C ABI for the target. This kind of struct should only be used for compatibility with the C ABI.

extern struct does not actually guarantee any fixed layout, and even if it did, you'd have to know some details about the C ABI to be sure.

The expectation that packed struct should behave like _attribute__((packed)) is not just an accident of history. It actually makes sense. Hence I'd be very much in favor of reintroducing byte-packed struct semantics alongside bitfield, possibly with a syntax like packed(.bit), packed(.byte), packed(.C) as suggested by @tleydxdy in the other thread.

Yes, there are things to clarify. extern struct actually won't depend on C ABI if you don't use C types. This is kind of unwritten rule as of now. packed struct makes a compact memory representation and it doesn't guarantee memory layout. While I still do not agree that packed struct and attribute((packed)) should mean the same thing, I do think we can do better by clarifying which semantics are for what purpose. For example default packed struct is compact representation of a struct by bitpacking, extern struct is for C ABI compatibility and/or guaranteed memory layout.

My proposal is to make clarify that extern can also be used for guarenteed memory layout, packed is NOT attribute((packed)), and for alignning to 1 you should do extern align(1). I don't see how changing packed or extern keywords for struct would somehow reduce confusion.

edit: And I think that making "packed(.bit), packed(.byte), packed(.C)" WILL FURTHER more cause confusion because for aligning semantics we already have align()

praschke commented 1 year ago

Right now, a lot of people assume that packed struct follows similar rules as regular or extern structs, which isn't true anymore with stage2. Thus, a lot of confusion is created and people misuse packed structs accidently or try to solve problems with them where extern struct is the right hammer.

I think before changing the name based on peoples' confusion, the documentation for extern struct should perhaps first be changed from

This kind of struct should only be used for compatibility with the C ABI. Every other use case should be solved with packed struct or normal struct.

With such a skeletal and foreboding warning, I don't think it is any wonder people think packed struct is what you should use for defining file headers and such.

EDIT: I will admit, I wouldn't be against this proposal if bitunion wasn't such a terrible name. The documentation needs to be fleshed out either way.

candrewlee14 commented 1 year ago

How about just renaming packed to bitfield? Then it’d bebitfield struct and bitfield union, with no bitunion.

eLeCtrOssSnake commented 1 year ago

How about just renaming packed to bitfield? Then it’d bebitfield struct and bitfield union, with no bitunion.

bitfield and bitunion are horrible names.

dee0xeed commented 1 year ago

Andre Weissflog said on zig discord that he mistook packed struct with attribute((packed)) in C

me too.

dee0xeed commented 1 year ago
// Equivalent of C's __attribute__((__packed__))
const S = extern struct {
    a: u64 align(1),
    b: u8 align(1),
};

__attribute__((__packed__)) means there is no spaces between u64 and u8 fields, so the total size is 9 bytes. It does not tell nothing about how an instance of this struct is aligned in RAM, right?

extern keyword as a word is neither about packing nor about alignment and thus is a reason of confusion - 'combine extern and align?..'

btw, hello to middle endian byte order! :)

nektro commented 1 year ago

related: #13009

jecolon commented 1 year ago

unpadded struct bit struct

InKryption commented 1 year ago

How about:

eLeCtrOssSnake commented 1 year ago

Maybe we should just clarify what packing we need when using packed struct: // My bit packed struct const MyStruct = packed(.bit) struct { ... };

// My byte packed struct const MyStruct = packed(.byte) struct { ... };

So packed becomes sort of modifier to struct. Or alternatively make packed accept the number of bits instead of .bit and .byte

praschke commented 1 year ago

i think bit struct and bit(u8) union, etc feels fine enough to me to just replace packed with bit. there's no need to squish it into one word.

dmbfm commented 1 year ago

One thing that really got me confused is that according to the documentation for extern struct:

This kind of struct should only be used for compatibility with the C ABI. Every other use case should be solved with packed struct.

But this seems to be false. There are use cases of extern structs which are not only for C ABI compability. For instance if I have a color struct:

const Rgb = struct {
   r: u8,
   g: u8,
   b: u8,
};

and I want it to be so an array [_] Rgb be densedly packed in memory (so that I can @memcpy or read directly), then, according to the documentation the regular struct does not give any guarantees as to layout, and extern struct is only for ABI compability. This led me to think that packed struct was the answer in this case; but it was not, since when using packed struct I get @sizeOf(Rgb) = 4 (it adds a one byte padding).

I don't really care about the naming that much, but would be nice if the documentation was more specific in this instance.

eLeCtrOssSnake commented 1 year ago

One thing that really got me confused is that according to the documentation for extern struct:

This kind of struct should only be used for compatibility with the C ABI. Every other use case should be solved with packed struct.

But this seems to be false. There are use cases of extern structs which are not only for C ABI compability. For instance if I have a color struct:

const Rgb = struct {
   r: u8,
   g: u8,
   b: u8,
};

and I want it to be so an array [_] Rgb be densedly packed in memory (so that I can @memcpy or read directly), then, according to the documentation the regular struct does not give any guarantees as to layout, and extern struct is only for ABI compability. This led me to think that packed struct was the answer in this case; but it was not, since when using packed struct I get @sizeOf(Rgb) = 4 (it adds a one byte padding).

I don't really care about the naming that much, but would be nice if the documentation was more specific in this instance.

Yes, this was basically my idea. I personally like the naming, just the docs need to be adjusted.

BogdanTheGeek commented 1 year ago

I also think packed is not representative of what its actually doing. I don't know @andrewrk 's opinion on what types and attributes should be in zig, but here is an idea from someone who has to do memory mapped devices with 14bit words every other day.

If types are better than attributes:

So putting something in a packet makes it "packed". Packets don't care about alignment, if it needs to be aligned to some something, it will have padding, otherwise it will be compact. Arrays with also work just fine as they will be aligned to their size. The only issues are see with this are:

The other side of the coin would favour attributes:

So, to get the equivalent of a packet, one would use compact ordered struct. A compact union is the same as a packed union A compact struct is a bitfield

The problems I can see with this solution are:

I personally would prefer first class types for this. I think packet is the natural evolution of C bit unions and packed structs and padding would be a god send for embedded. With the risk of this being considered a manifesto, here is a real life example for this, it should be very easy to read even if you are not an electrical engineer:


// https://www.mouser.co.uk/datasheet/2/268/MCP7940N_Battery_Backed_I2C_RTCC_with_SRAM_2000501-2937048.pdf
// MCP7940N RTC with battery backup

const I2C_Command = bitfield {
    address: u7,
    write: bool,
}

const I2C_Packet = packet {
    command: I2C_Command,
    data: u8,
}

const MCP7940N_AlarmMask = enum u3 {
    SECONDS = 0,
    MINUTES = 1,
    HOURS = 2,
    DAY = 3,
    DATE = 4,
    RESERVED1 = 5,
    RESERVED2 = 6,
    ALL = 7,
}

// Could be a bitfield so padding would be implicit, but this is more explicit
const MCP7940N = packet {
    sec: bitfield {
        value: u7,
        status: const bool,
    },
    min: bitfield {
        value: u7,
        const padding = 0,
    },
    hour: bitfield {
        union {
            bitfield {
                value12: u4,
                pm: bool,
            },
            value24: u5,
            24hr: bool,
        },
        const padding = 0,
    },
    day: bitfield {
        value: u3,
        vbat: const bool,
        pwrfail: const bool,
        oscrun: const bool,
        [2]const padding,
    },
    date: bitfield {
        value: u6,
        [2]const padding,
    },
    month: bitfield {
        value: u5,
        leap: bool,
        [2]const padding,
    },
    year: bitfield {
        value: u8,
    },
};

I could have defined the default register values too, but this is just a quick demo. This would also address https://github.com/ziglang/zig/issues/13009

nmichaels commented 1 year ago

The whole argument is that somebody confuses packed with C packed and now suddenly we have to change it. But we're not C. We're a different language.

I think the criticism that C's packed and Zig's packed mean very different things is entirely valid. zig.org's front page suggests adding "a Zig compilation unit to C/C++ projects." It makes sense that we might acknowledge that features in C have names.

Some things in C (like calling private things static, and having things be public by default) were, in Zig's opinion, mistakes. That's fair. I agree. However, the word "packed" for structs that don't have any padding seems pretty sensible to me. I'm not sure we should consume the valuable friction we're allotted by having it mean something entirely different.

zxubian commented 1 year ago

I think the core issue is not just that "packed" in Zig means something different to C. It's that

  1. having a struct with no padding between fields (a tightly-packed struct, if you will) is a common requirement for a low-level language
  2. the current way of achieving this very common use-case in Zig (extern struct & align(1)) is unintuitive, has poor readability and discoverability both for people familiar with C, and those not familiar.

Personally, when I want to control the memory layout of a struct, I reach for words like "layout", "padding", "packing" etc. I don't immediately think of "extern".

Conversely, I think the code snippet by @BogdanTheGeek above was very easy to read.

dmbfm commented 1 year ago
  1. the current way of achieving this very common use-case in Zig (extern struct & align(1)) is unintuitive, has poor readability and discoverability both for people familiar with C, and those not familiar.

And the documentation makes things even more confusing by saying that:

This kind of struct should only be used for compatibility with the C ABI. Every other use case should be solved with packed struct or normal struct.

Which simply isn't true, since you use extern struct to get a struct without padding, which has more uses than just C interop.

nmichaels commented 1 year ago

And the documentation makes things even more confusing by saying that:

I think we're all agreed that the documentation is misleading and confusing, but maybe it shouldn't be. Maybe extern struct should really only be for C interoperability, in which case I think we need a new thing (or to re-repurpose packed) for the common use case of a struct whose job is to map to some sort of data structure that's represented outside of just in-memory.

Whether the name for that is unpadded, packet (while I like @BogdanTheGeek's suggestion I don't love the word packet), compact, packed, or some as-yet unsuggested but brilliant word, I think it's important to get agreement that this is a language feature we want to have, distinct from extern struct with align(1). Or maybe it's just sugar.

Personally, I think packed is a good name for it, and the thing that's currently called packed, if it needs to exist, should get a different one. Then we can leave the extern struct documentation alone and write a whole bunch of other documentation.

kj4tmp commented 4 months ago

I also hit this confusion when trying to understand the expected behavior of translate-c when encountering a #pragma pack

https://github.com/ziglang/zig/issues/20405

Cloudef commented 3 months ago

I agree with this, there should be a clear distinction between bitpacked and bytepacked.