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

nfields is easily misused and can lead to bugs #29977

Open
KristofferC opened this issue Nov 9, 2018 · 6 comments
Open

nfields is easily misused and can lead to bugs #29977

KristofferC opened this issue Nov 9, 2018 · 6 comments
Milestone

Comments

@KristofferC
Copy link
Sponsor Member

Here is a struct for a bitmap header

struct BitmapHeader
    signature               :: UInt16
    bmp_size                :: UInt32
    reserved_1              :: UInt16
    reserved_2              :: UInt16
    imag_data_offset        :: UInt32
    size_bitmap_info_header :: UInt32
    width                   :: UInt32
    height                  :: UInt32
    n_planes                :: UInt16
    bits_per_pixel          :: UInt16
    compression_type        :: UInt32
    image_size              :: UInt32
    horizontal_resolution   :: UInt32
    vertical_resolution     :: UInt32
    n_colors                :: UInt32
    n_important_colors      :: UInt32

    r_mask                  :: UInt32
    g_mask                  :: UInt32
    b_mask                  :: UInt32
    meh                     :: UInt32
end

We want to compute the packed size (no padding) so we write something like

function sizeof_packed(h::Type{BitmapHeader})
   sum(sizeof(fieldtype(BitmapHeader, i)) for i in 1:nfields(BitmapHeader))
end

and it seems to work fine

julia> sizeof_packed(BitmapHeader)
70

There is however a terrible bug here which can be made obvious with:

julia> struct Foo end

julia> nfields(Foo)
20

😱 We are getting the number of fields inside the DataType object (which in this case happened to be the same number as the fields inside BitmapHeader.) Adding fields to BitmapHeader will now lead to (likely silent bugs).

The correct function we should have used is fieldcount.

julia> fieldcount(Foo)
0

The problem here is:

  • It is hard to know in general whether reflection-functions operates in the value or type domain. We now for example have isbitstype and isbits that helps with this. Having nfields(x) and fieldcount(x) give different answers is error prone.
  • nfields(::DataType) is just almost never what you want. These are internal fields so having some internal function for getting that count wouldn't be too bad.
@KristofferC KristofferC added this to the 2.0 milestone Nov 9, 2018
@JeffBezanson
Copy link
Sponsor Member

This is why there are doc strings. Ok to propose new names for these though, which can even be introduced in 1.x.

@StefanKarpinski
Copy link
Sponsor Member

Why not just delete nfields? fieldcount(typeof(x)) is the unambiguous way to do this.

@JeffBezanson
Copy link
Sponsor Member

That would prevent us from having variable length objects.

@StefanKarpinski
Copy link
Sponsor Member

Ah, I see. That makes sense.

@o314
Copy link
Contributor

o314 commented Aug 20, 2019

Are this kind of expando so common ?

The only references to variable length objects i found in julia github were for simplevector and interop with fortran string ... Not so many it seems

@StefanKarpinski
Copy link
Sponsor Member

Not so many it seems

Currently.

cmcaine added a commit to bovine3dom/Nauty.jl that referenced this issue Nov 24, 2021
TODO: learn to read docstrings

See also JuliaLang/julia#29977
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

4 participants