Add special syntax for padding in lib's structs

[bew]

Even if those names could be generated, I think it would be better if we don't even have to think about them.

I was thinking about allowing something like this for a start:

lib LibFoo
  struct Bar
    field1 : Int32
    _ : UInt8[3] # skip 3 bytes
    field2 : UInt8
  end
end

But there's still 2 downside I see here with this idea:

• _ is not very explicit that this is for padding, it's just that I don't want to specify a name because I don't need this part of the struct.
• UInt8 is redundant I think, because a binary padding is almost always expressed in number of bytes (sometimes in number of bits)

Maybe we could find a more explicit way? like a macro padding(bytes: 3) in the struct's body?

What do you think?

[asterite]

What's wrong with padding : UInt8[3]? We are not adding new syntax just for this.

[bew]

Now that I re-think about it, a "new syntax" is probably overkill, but I still think that allowing the use of _ for the field name to mean "I don't need the name of this field" is acceptable (and shouldn't be a big change).

For example some lib have padding at multiple places:

lib LibXCB
  struct GenericEvent
    response_type : UInt8
    _padding : UInt8
    sequence_number : UInt16
    _more_padding : UInt32[7]
    full_sequence : UInt32
  end
end

Here I have to give a name to the 2 padding fields like _padding & _more_padding (or padding1, padding2). I think it would be easier & cleaner to just not have to specify the name of a field we don't need, but that we still need to put in there for the space it uses.

I think it is cleaner this way:

lib LibXCB
  struct GenericEvent
    response_type : UInt8
    _ : UInt8
    sequence_number : UInt16
    _ : UInt32[7]
    full_sequence : UInt32
  end
end

Also, in the latter you directly see the relevant fields.

[ysbaddaden]

I'm closing as most won't fix/implement.

lib definitions are meant to written once (if not automatically generated) from C headers, then wrapped in nice Crystal structs. Nobody should have to look them up often enough that a __pad1 field name is gonna be a bother to look at.

[Sija]

Closing already? I'd think it would be coherent with how _ underscore var is used for method/proc arguments.

[ysbaddaden]

As you said: blackhole is for arguments, not methods.

[bew]

To me _ underscore is a special identifier that can be used when we don't need a variable/type, but you still need to specify it (consequence, for a variable it's a blackhole, it can't be used at all).

In this case the wanted terminology looks the same: we don't need the field, but we still need to specify it. Consequence there won't be any getter/setter generated for this field, it can't be used at all, but it's still there. It feels coherent to have _ for that.

[RX14]

I also think this change would make sense, but it is low-priority. It's a relatively minor change and it's probably worth discussing more.

[ysbaddaden]

Change is minor, but suddenly lib struct fields may not have a name anymore, that may have bigger consequences than expected for such a "minor" change.

• This is for lib's struct only:
  • they should be generated automatically (see clang.cr), and a generator can't distinguish padding;
  • Crystal struct / class won't benefit from this;
• Lib structs aren't documented;
• You should never have to deal with lib structs;
• 2 contributors believe the benefit is insignificant, and not worth the hassle.

Why bother keeping such an issue opened? Hence I closed.

[Sija]

• This is for lib's struct only:

ATM it's the only use-case where setting padding would be convenient, so that's not an issue.

• they should be generated automatically (see clang.cr), and a generator can't distinguish padding;

Using _ as a padding would be an option, not a requirement, not an issue either.

• Crystal struct / class won't benefit from this;

See point no. 1, not an issue for Crystal-land.

• Lib structs aren't documented;

They are not documented and will not be, so what's the problem here? What does it have to do with the issue at hand?

• You should never have to deal with lib structs;

?

• 2 contributors believe the benefit is insignificant, and not worth the hassle.

OTOH 3 contributors found it possibly useful, at least to the point of discussing it in greater length than You.

Why bother keeping such an issue opened? Hence I closed.

Because aside Your opinion there's enough interest in keeping it open for further discussion?

[ysbaddaden]

This is a new syntax for an optional sugar that's only useful in a single scenario: the developer of a shard wrapping a C library that manually writes lib bindings and doesn't like writing or seeing _pad1 in struct field names.

It's useless in usual crystal code, in documentation, and so on, it's not even useful for generated bindings.

[Sija]

If I understand correctly, that's exactly the use-case this issue is aiming for.

[straight-shoota]

There hasn't been any interest in this for three years. I think it's mute to introduce a new syntax for something that already works and won't yield a significantly different result. Lib definitions are only used internally, so there's no real need for compiler support to limit access to unusable properties. For the purpose of signifying a property as meaningless padding you can express that in the name and/or via a source code comment.

[HertzDevil]

IMO there is a need to be able to annotate a lib struct field as padding, to signal that it should be excluded in equality comparisons.

Equality between lib structs, provided by Struct#==(other) : Bool, compares all instance variables pairwise, but it's impossible to redefine methods in lib types. Thus equality works when the padding fields are omitted:

lib L
  struct Foo
    x : Int8
    y : Int32
  end
end

struct Struct
  def ==(other) : Bool
    p! self  # => L::Foo(@x=1, @y=33752069)
    p! other # => L::Foo(@x=1, @y=33752069)
    previous_def
  end
end

a = L::Foo.new
a.x = 1_i8
a.y = 0x02030405

b = L::Foo.new
pointerof(b).unsafe_as(Pointer(Int32)).value = 0x01010101
b.y = 0x02030405

a == b # => true

But if we provide the padding:

lib L
  struct FooPadded
    x : Int8
    __padding : UInt8[3]
    y : Int32
  end
end

or a specific C binding requires it, then a == b becomes false. This happens to LibC::Stat for example, and leads to random CI failures. To make this pass we have several alternatives:

• Redefine Crystal::System::FileInfo#== for every target triple, making sure the padding fields are not compared. (This type is of course not the LibC::Stat itself, but a Crystal struct containing a LibC::Stat as an instance variable.)
• Reset all padding fields every time after a lib struct is passed to and from C. We cannot assume the C code always leaves the padding fields intact.
• Define LibC::Stat as an @[Extern] Crystal struct so that we could override its equality method. This is not applicable to all lib structs, because their instance variables lose automatic #to_unsafe conversions.
• Allow fields like __padding to be annotated, then modify Struct#== so that it ignores any instance variables with such an annotation. (This could even benefit non-lib Crystal structs if we name the annotation with something other than "padding". It is only that lib structs need it more than non-lib ones because of the inability to redefine #== for the former.)

Of these approaches the last alternative is the most flexible, least error-prone, and one that truly exercises Crystal's metaprogramming capabilities.

[asterite]

it's impossible to redefine methods in lib types

It's actually possible: reopen the type outside of the lib and define methods on it ;-) (just note that this is an accident of how things are implemented)

[asterite]

So if an equality needs to be defined for such struct right now to solve a bug, you can use that trick.