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

New idl for testing setting the MemberId [20109] #17

Merged
merged 3 commits into from
Dec 11, 2023
Merged

Conversation

richiware
Copy link
Member

No description provided.

Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
IDL/appendable.idl Show resolved Hide resolved
IDL/final.idl Show resolved Hide resolved
IDL/key.idl Show resolved Hide resolved
Comment on lines 4 to 5
@id(100)
octet o;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I think it is easier to read if the annotation and the type are in the same line as it is usually written in the specifications. Otherwise, it might be helpful to include a blank line between types. Though this is only a style suggestion and as there is no IDL code style, it can be overlooked.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 65376cd

Comment on lines 117 to 128
@autoid(HASH)
struct DerivedAutoidHash : AutoidHash
{
char cd;
@id(101)
octet od;
@hashid
short sd;
@hashid("long2")
long ld;
long long lld;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a specific inheritance test case that it would be nice to test as well. What happens if there is nested inheritance as the following IDL:

struct SomeStruct
{
    short some_var;
};

struct EmptyStruct : SomeStruct
{
};

struct AnotherInheritedStruct : EmptyStruct
{
    long another_var;
};

Probably the logic in IDL Parser would handle this case correctly, but I suggest having the test case to ensure that in the future is not broken.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 65376cd

@richiware richiware changed the title New idl for testing setting the MemberId New idl for testing setting the MemberId [20109] Dec 11, 2023
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JLBuenoLopez JLBuenoLopez merged commit bc2dd38 into main Dec 11, 2023
1 check passed
@JLBuenoLopez JLBuenoLopez deleted the feature/autoid branch December 11, 2023 08:44
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

Successfully merging this pull request may close these issues.

2 participants