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

Offsets using parameters should not be allowed #647

Open
mikir opened this issue Jul 2, 2024 · 3 comments
Open

Offsets using parameters should not be allowed #647

mikir opened this issue Jul 2, 2024 · 3 comments
Labels
core Zserio core module enhancement New feature or request schema language compatibility This can break backward compatibility of schema language
Milestone

Comments

@mikir
Copy link
Contributor

mikir commented Jul 2, 2024

Consider the following schema:

struct OffsetHolder
{
    uint32 roomOffset;
};

struct Room(OffsetHolder offsetHolder)
{
offsetHolder.roomOffset:
    uint16 roomId;
};

struct School
{
    uint16 schoolId;
    OffsetHolder offsetHolder;
    Room(offsetHolder) room;
};

Such schema is completely valid but it prevents to pass parameters using const references (parameter offsetHolder in Room cannot be passed by const reference). This brings significant complexity for implementation of generators.

Because we cannot see any practical usage of this feature, it would be probable the easiest solution just to disable it. Perhaps, it will be enough to define offsets only where there are used:

struct Room
{
    OffsetHolder offsetHolder;
offsetHolder.roomOffset:
    uint16 roomId;
};

struct School
{
    uint16 schoolId;
    Room room;
};
@mikir mikir added enhancement New feature or request core Zserio core module labels Jul 2, 2024
@mikir mikir added this to the 2.15 milestone Jul 2, 2024
@mikir
Copy link
Contributor Author

mikir commented Aug 6, 2024

This is a little bit controversial because it really could break the backward compatibility for some user schemas. We will have to rethink it twice if we can afford to do it in this milestone.

@mikir mikir added the schema language compatibility This can break backward compatibility of schema language label Aug 9, 2024
@mikir
Copy link
Contributor Author

mikir commented Aug 12, 2024

We will implement it only if this is absolutely necessary for new C++17 generator.

It has been moved to the backlog in the meantime.

@Mi-La
Copy link
Contributor

Mi-La commented Aug 21, 2024

If parameters will remain allowed in offset expression, we should create a new issue for checking duplicated use of offset fields - see #645 where only simple cases were covered.

Consider following schema:

struct Holder
{
    uint32 offset;
};

struct Parameterized(Holder holder)
{
    uint32 array1[];
holder.offset:
    string array2[];
};

struct Container
{
   Holder holder1;
   Holder holder2;
   Parameterized(holder1) param1; 
   Parameterized(holder2) param2; // this is ok since another instance of holder is used
   Parameterized(holder1) param3; // !!! here holder1.offset will be used again !!!
};

struct OtherContainer
{
    Holder holder;
holder.offset:
    Parameterized(holder) param; // !!! here holder.offset will be used again !!!
};

The problem is that we don't track instances of compound fields and thus we cannot check whether the same offset field is used multiple times or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Zserio core module enhancement New feature or request schema language compatibility This can break backward compatibility of schema language
Projects
None yet
Development

No branches or pull requests

2 participants