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

Nesting FieldFlags Types #13

Open
mrufsvold opened this issue Jun 20, 2023 · 11 comments
Open

Nesting FieldFlags Types #13

mrufsvold opened this issue Jun 20, 2023 · 11 comments
Labels
feature New feature

Comments

@mrufsvold
Copy link

It would be helpful if fields of a @bitfields struct could be type annotated with types generated by FlagFields so that you could nest types for cleaner code organization without losing the tight packing.

For example:

@bitflags mutable struct BasicNeeds
       hungry
       tired
       injured
end

@bitfields mutable struct Health
    basic_needs::BasicNeeds
    disease_status:2
ebd

struct Person
    health::Health
end

person.health.basic_needs.hungry = true

Could be considered once #9 is implemented.

@Seelengrab Seelengrab added the feature New feature label Jun 20, 2023
@mrufsvold
Copy link
Author

I definitely don't have the bandwidth for several months, so I wouldn't wait on a PR from me, but if you don't get around to this, I think I could get it working with the "register new types within FieldFlags" solution you suggest on Discourse.

My naive thought would be a table that stores Module, Name, and BitWidth each time the macro runs. Then, if it finds a type annotation, look there first before falling back to a generic constructor (i.e. for UInt16).

Is there other metadata besides those three that would be needed to construct the field?

@Seelengrab
Copy link
Owner

Seelengrab commented Jun 20, 2023

Just that unfortunately isn't enough - for example, when someone has a UInt16 type shadowing the one from Base, I wouldn't want them to not be able to access that. So just storing a map like :UInt16 => UInt16 (the symbol to the type) is IMO too fuzzy 🤔 The only way around this I can see is using eval of the module the macro runs, and storing the result of that in some cache.. Not a too pretty solution, I'll have to think about it some more.

@mrufsvold
Copy link
Author

Sorry, I was ambiguous!

The table I'm proposing would carry metadata only for types defined using FieldFlags.

For all other isbits types, whatever solution you come up with for #9 would be the fallback, but to tightly pack FieldFlags types inside a struct, we'd need to know 1) that the type was defined by a FieldFlags macro and 2) the number of bits to allocate for the field.

So the check is something like:

  1. Do getproperty(__module__, type_name).
  2. Check if the type is in the field_flag_types_table.
  3. If so, how many bits does it need?
  4. Use the result of 1 to make a constructor for interpreting the stored bits.

If it isn't in the table, fall back to #9.

I might be full of it here; I'm away from my computer, so I can't test this theory at the moment. But I just wanted to clarify that I wasn't suggesting a map of Symbol => Type.

One question this raises is if a @bitflags type constructor could accept a UInt and use its least significant bits to fill the fields.

@Seelengrab
Copy link
Owner

Seelengrab commented Jun 20, 2023

The issue is that this needs a map Symbol => Type in the first place, to be able to figure out which type you'd need to look at to get the correct offset :) For example

module A

@bitflags struct Foo
    a
    b
end

end

module B

@bitflags struct Foo
    a
end

end

Both modules define a Foo with @bitflags but with different sizes, so the only way to distinguish the two in a macro is to remember where each Foo is defined and have that be part of the lookup. That's the same infrastructure required for annotating UInt16 though, since you need to be able to distinguish Base.UInt16 and MyModule.UInt16, if MyModule contains a definition shadowing the one from Base (and if it doesn't, annotating with UInt16 needs to give the one from Base, hence the need for local eval in the macro...).

(Un)Packing the object is the easy part in all of this; that's just a lookup of the object type and a conversion, similar to how a:3::UInt16 would zero-extend. That's why I'd rather do #9 first, since that will require the same kind of infrastructure, but with fewer tricky edgecases.

One question this raises is if a @bitflags type constructor could accept a UInt and use its least significant bits to fill the fields.

That's IMO an orthogonal question. This usecase is also already supported with convert(T, 1) and should not be done through the constructor directly.

@Seelengrab
Copy link
Owner

Seelengrab commented Jun 22, 2023

To summarize, there are two parts to this:

  1. Getting the translation Symbol -> Type right, which should be solved by Custom field type annotations #9
  2. Deciding how to store the information that a type is from FieldFlags.jl

The second part is likely solved by a traits-like approach (which I'd REALLY prefer to be a subtyping query, but alas, we don't have multiple abstract inheritance...).

So something like

isFieldFlagsType(::Any) = false

with per-type defined overrides to true, generated by @bitflags and @bitfield. During construction, we can then check the type we got from 1. for isFieldFlagsType and pack accordingly, omitting trailing padding.

@mrufsvold
Copy link
Author

Here is a MWE of what I was describing above:

module A
# Keep track of types we made
const Type_Table = Tuple{Type, Int64}[]
const Missing_Row = (Any, 0)
update_table(::Type{T}, width) where T = push!(Type_Table, (T, width))

# check if the name/module combo for this call already exists
function check_for_type(mod, name)
    fail_response = (false, Missing_Row)
    name in names(mod; all=true) || return fail_response
    obj = getproperty(mod, name)
    obj isa Type || return fail_response
    matching_row_i = findfirst(row -> row[1] === obj, Type_Table)
    matching_row_i === nothing && return fail_response
    return (true, Type_Table[matching_row_i])
end

macro make_type(name, width)
    check_result = check_for_type(__module__, name)
    @show check_result
    if check_result[1]
        println("I already made $name in this module! It is $(check_result[2][2]) wide.")
        return :()
    end
        
    esc_name = esc(name)
    return quote
        struct $esc_name end
        update_table($esc_name, $width)
    end
end
end

module B
import ..A
@A.make_type T1 5
@A.make_type T2 10
# Check catches we already defined this type
@A.make_type T1 6
end

module C
import ..A
import ..B
import ..B: T2
# no conflict because B.T1 !- C.T1
@A.make_type T1 5
# Fails because T2 was imported
@A.make_type T2 5
end

Also, not sure if this helps in #9, but getproperty(C, :UInt8) works in the example above, so the A.check_for_type logic could be extended to provide the Symbol => Type map for non-FieldFlags types.

@mrufsvold
Copy link
Author

mrufsvold commented Jun 22, 2023

omitting trailing padding

I didn't realize the "real" bit width was also stored in bitflag types (I only saw getting the actual memory layout width--rounded up to a number of bytes-- documented)!

With that in mind, I think you're absolutely right that a traits-style solution to problem 2 in your response makes the most sense. The table is redundant if we don't need to store metadata.

@Seelengrab
Copy link
Owner

Seelengrab commented Jun 22, 2023

Yes - as long as the macro can infer all sizes at macro expansion time (which is required anyway - we don't currently have variable sized structs in julia, at least in any manner that's convenient to use & implement with little to no overhead), all of this offset/bitwidth business can be hardcoded per-type. That's also exactly how the package works right now and is why only literal sizes are supported.

The true bitwidth isn't actually stored, but it can be trivially computed by calculating the difference between the last bit position of the last field and 8*sizeof(T).

@Seelengrab
Copy link
Owner

Seelengrab commented Jun 22, 2023

Ok, one thing I've noticed - something like nesting a mutable in anything is not going to work, because person.health.basic_needs.hungry = true lowers to this:

julia> Meta.@lower person.health.basic_needs.hungry = true
:($(Expr(:thunk, CodeInfo(
    @ none within `top-level scope`
1%1 = Base.getproperty(person, :health)
│   %2 = Base.getproperty(%1, :basic_needs)
│   %3 = true
│        Base.setproperty!(%2, :hungry, %3)
└──      return %3
))))

and the result of that last setproperty! semantically isn't stored back into the container explicitly (it doesn't need to be - the object is assumed mutable after all, which is true, but that also means storing it in line with the other data in the wrapper struct can't really be done). This is also the reason why something like Accessors.jl exists, which handles the reconstruction of the immutable for you, and is why you need @set or something like that.

There could be a way to do it, if a.b doesn't actually return the object itself, but rather something representing the object, to store any later setproperty! calls in the original outer object.. The issue then is that this object isn't of the correct type, so dispatching on it won't work as expected.

Immutables can work fine though, since you need to replace them with a new instance explicitly anyway.

I guess that means you'll need some other way to make your fields shared between objects? Maybe some macro emitting the shared fields of an object, so they can be accessed directly? I'd have to add support for looking into quoteblocks in @bitflags and @bitfield, but that sounds doable.

@mrufsvold
Copy link
Author

Ah, yeah, huh! I'm glad you looked at that. I'll ponder and get back to you when I have a decent proposal.

@Seelengrab
Copy link
Owner

Sounds good! I'll leave this issue open though, since storing primitive types and immutable bitstypes that are padding free is possible. It's certainly lower on the priority list though, the usecases for that exact thing seem rather niche.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

No branches or pull requests

2 participants