wc-duck / datalibrary

Open Source Data Library for data serialization.
Other
42 stars 8 forks source link

Add support for unions. #46

Closed wc-duck closed 8 years ago

wc-duck commented 8 years ago

Support for union types has been requested from both users and myself, so that should be added. Here is my suggestion for interface.

typelib:


    {
        // add new section "unions" to typelib.
        "unions" : {
            "my_union_type" : {
               // this would be the actual name of the union, see below. ( not the best name for the key =/ )
               "name" : "v", 

                // defined as dict, it could also be defined as a [] to match "types"?
                "members" : {
                    "member_1" : "other_defined_type1",
                    "member_2" : "other_defined_type2"
                }
            }
        }
    }

this would generate this type-header

struct my_union_type
{
    dl_typeid_t type; // type currently present in union
    union
    {
        other_defined_type1 member_1;
        other_defined_type2 member_2;
    } v;
};

However I have no really good idea for how to specify this in the text format yet =/ Lets say that we have a type with a member "union_member" of the above defined type, like this:

struct the_type
{
    union_type union_member;
};

how would that be specified in text-data ( how would the text-data specify what member to actually initialize? Maybe like this?

{
    "the_type" : {
        "union_member" : {
            "member_1" : {
                // ... other_defined_type1 data here ...
            },
            // ... setting more than one member would be an error ...
        }
    }
}
lundmark commented 8 years ago

Hmmm, what you would need is something that generates something like this:

struct the_type
{ 
   union_type union_member;
   uint32 union_member_id;
};

So that you can distinguish between which actual type it is... I'll ponder a bit about how to get this done nicely in the text-data.

wc-duck commented 8 years ago

Isn't that exactly what I have proposed? dl_typeid_t is a uint32_t typedef:ed in dl.h?

wc-duck commented 8 years ago

Hmm... I see what you say now, generate unions in the same way as arrays, just as an inline struct.

    struct mytype
    {
        struct 
        {
            dl_typeid_t type;
            union
            {
                 // ...
            }; // anonymous?
        } union_member
    }
lundmark commented 8 years ago

Yes... exactly. I think that's needed for runtime analysis of what the actual union is.

I think that the union (in my usage case anyway) should just be setup between different dl types (maybe support default types as well...):

{
    "my_union_type" : {
        "union_type_1",
        "union_type_2"
    }
}

should suffice?

EDIT: This will naturally be under the "unions"-section as you specified.

wc-duck commented 8 years ago

That might suffice, but it "closes" the api for adding other info to the union. Would you need to set "alignment" on the union, maybe set that it is "extern" ( both can be set on type )? Without the "members" key you couldn't add a "comment" in the same way as on types.

I am leaning towards that version since it's more compact, just that I do not want to change it later if one find that it is not enough. In your case it would be:


    "unions" : {
        "my_union_type" : {
            "member_1" : "some_dl_type1",
            "member_2" : "some_dl_type2",
        },
    }
wc-duck commented 8 years ago

I'm also thinking about the anonymous union in the generated c code... that is unfortunatly not valid without disabling a warning on visual studio =/

One could generate a disable of the warning, but that's not really nice =/

i.e.


struct some_struct
{
    dl_typeid_t type;
    union
    {
        some_dl_type1 member1;
        some_dl_type2 member2;
    }; // <--- no name here generates warning on msvc =/
};

I would rather have dl generate warning-free code, but maybe a flag on how to generate the header? Or just accept that there need ot be a .data to access the union?

such as:


if( instance.type == SWhooType::TYPE_ID )
    do_stuff( instance.data.whoo_member );
lundmark commented 8 years ago

Ah I see. Yes that makes sense (members so that more stuff can be added like tags etc).

I think that generating warning-free code is more important than having the union nameless - the .data-access is definitely acceptable!

wc-duck commented 8 years ago

As a small update I have unions working locally, just need to add some tests and cleanup the implementation a bit, will hopefully submit something rough during the week.

wc-duck commented 8 years ago

First pass submitted, I need to write more tests and there is an issue with ordering of types when generating header-files that need to be solved. But it might be testable right now.

wc-duck commented 8 years ago

Lets call this done and add new issues for the things that fail :)