repair issue #461

[ruoruoniao]

No description provided.

[eboasson]

Thank you @ruoruoniao for looking into #461 (I seem to have forgotten it ...). I am surprised that this change would fix it, though because the difference between idl_is_optional and is_optional seems to be that:

• idl_is_optional checks whether this "node" is marked as optional,
• is_optional walks the typedef chain to see if any of those nodes are marked as optional,
  and but is_selfcontained itself also walks that typedef chain, and so I would expect the two to behave the same.

Could you please help me understand?

Two other small things:

• It is much easier for me (or anyone trying to find a correlation between a problem and a fix) if the commit subject and the PR title briefly explain what it is about, rather than just refer to a number. If the subject is something mentioning optionals and self-contained it'll be much easier.
• The Eclipse Foundation has a rule that commit authors must have signed the Eclipse Contributor Agreement before it can be merged.

[ruoruoniao]

Thank you @ruoruoniao for looking into #461 (I seem to have forgotten it ...). I am surprised that this change would fix it, though because the difference between idl_is_optional and is_optional seems to be that:

• idl_is_optional checks whether this "node" is marked as optional,
• is_optional walks the typedef chain to see if any of those nodes are marked as optional,
  and but is_selfcontained itself also walks that typedef chain, and so I would expect the two to behave the same.

Could you please help me understand?

Two other small things:

• It is much easier for me (or anyone trying to find a correlation between a problem and a fix) if the commit subject and the PR title briefly explain what it is about, rather than just refer to a number. If the subject is something mentioning optionals and self-contained it'll be much easier.
• The Eclipse Foundation has a rule that commit authors must have signed the Eclipse Contributor Agreement before it can be merged.

Well, I have an idl like this:

@final struct OptionalTest
{
    uint64 id;
    uint64 eventID;
    @optional uint64 errorID;
};

This structure member will convert to this:

{
    //node(OptionalTest)
    members: [
        { type_spec: node(id), ... },
        { type_spec: node(eventID), ... },
        { type_spec: node(errorID), ... }
    ],
    ......
}
{
    //node(errorID)
    mask: 1551,
    idl_is_optional(this)     : false,
    idl_is_member(this)     : false,
    idl_is_declarator(this)  : false,
    idl_is_type_spec(this)  : true,
    parent: node(optional), 
    ....
}
{
    //node(optional)
    idl_is_optional(this) : true,
    idl_is_member(this) : true,
    ....
}

So if call idl_is_optional, node(errorID) is not a member or a declarator, it cannot route to node(errorID)->parent. Also, add a if-else branch like:

...
else if (idl_is_type_spec(node)) {
    const idl_node_t *parent = ((const idl_node_t*)node)->parent;
    return is_selfcontained(parent);
}
...

will fix this question too.

[eboasson]

Thanks for the explanation! I completely forgot about the typedef-vs-type-spec thing. Yes, then it is obvious that it makes a difference.

I prefer your second version, where you keep the call to idl_is_optional and add

else if (idl_is_type_spec(node)) {
    const idl_node_t *parent = ((const idl_node_t*)node)->parent;
    return is_selfcontained(parent);
}

If you change it this way I'll be happy to merge it.

The reason is simply that the current version looks "off" and I expect it will cause people (me?) to go down the same rabbit hole again in the future. It won't solve the underlying problem that such mistakes are just too easy to make with the way types are represented internally in idlc, but fixing that is quite a project ... so for the time being the best we can do is fix the problems that crept in.