Unamed union member when map item is given a choice

[jnz86]

A couple derivative issues here.

I wanted to include a choice of value options for an item in a map. The generated code is a little off.

CDDL:

test_vars = (
    test_length : "l" : uint .le 4096,
    test_data   : "d" : bstr,
    test_hash   : "h" : uint / int,
)

pl_selftest = {
    test_vars
}

Generated header:

struct test_vars_r {
    uint32_t test_length;
    struct zcbor_string test_data;
    union {
        uint32_t uint;
        int32_t Int;
    };
    enum {
        test_vars_test_hash_uint_c,
        test_vars_test_hash_int_c,
    } test_hash_choice;
};

struct pl_selftest {
    struct test_vars_r test_vars_m;
};

Issues:

• "hash" is missing it's name as a union. It generates a test_vars_r.test_length, a test_vars_r.test_data.len and pointer, and then... It seems like test_vars_r.test_hash_uint should exist. Instead... test_vars_r.uint does, and that's not a very helpful variable name.

• In the above example, lowercase uint and starts uppercase Int are generated. That seems like a mistake even if I did have the name "hash" in there.

• If the choice is between uint and nil in the cddl, then in the header, a union with a single uint is generated. I see why this would happen, but it's not a very helpful union of one item. It's also not really harming anything so I get if it's a low priority.

(Also, as an aside, I am still not sure what the _r means, any help there?)

Using commit d1b4e0512c2783c5a4713f2b4b26c0d8c211b155

[oyvindronningstad]

Hi, thanks for taking the time.

• Since the name is auto-generated it's not going to be the most sensible. A lot of care has gone into the naming algorithm, and if I find a good way to improve this, I will.
• int is capitalized because it's a reserved keyword in C. I agree it looks funny here, maybe I will change it for cosmetic purposes.
• Here, I disagree. Keeping the union even with a single element makes it more obvious what elements are part of the union, and like you say it does no harm.
• "_r" is added to remove naming ambiguity. The meaning is not entirely obvious, and is more an artifact of the internal workings of the script.

There are no concrete plans to do anything about these, but I will keep it open for a while in case I e.g. do some naming fixes.

[jnz86]

No wait, I think you didn't see the issue. I'm not questioning that if there is a choice then an int / uint, that isn't named a way that I didn't find sensible. It's not named at all. Which is legal because it's an anonymous union - legal once, but not twice.

See this change to add a second one:

test_vars = (
    test_len   : "l" : uint 4..4096,
    test_data  : "d" : bstr,
    test_hash  : "h" : uint / int,
    test_hash2  : "h2" : uint / int,
)

truct test_vars_r {
    uint32_t test_len;

    struct zcbor_string test_data;

    union {
        uint32_t uint;
        int32_t Int;
    };

    enum {
        test_vars_test_hash_uint_c,
        test_vars_test_hash_int_c,
    } test_hash_choice;

    union {
        uint32_t uint;
        int32_t Int;
    };

    enum {
        test_vars_test_hash2_uint_c,
        test_vars_test_hash2_int_c,
    } test_hash2_choice;
};

The first one is allowed to be an anonymous union. Where it should have taken the union name of test_hash in my opinion but did not. The second one is an error in C, duplicate member uint/Int, but of course won't be if it was named test_hash2.

This seems unintentional, no?

[oyvindronningstad]

Ah ok. I assume you're using --short-names. Currently --short-names is not guaranteed to give unique names, so the workaround for that is changing the CDDL or not using --short-names.

[jnz86]

I see.

I’ll look into not using short names for this module. More importantly, I seems clear to me what the fix is here, that hash probably needs a name either way.

But, since no one wants me mucking about in their Python code. Best I can do is to point it out.