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

_create builder methods should have arguments in id order #81

Closed
mikkelfj opened this issue Mar 1, 2018 · 2 comments
Closed

_create builder methods should have arguments in id order #81

mikkelfj opened this issue Mar 1, 2018 · 2 comments
Labels

Comments

@mikkelfj
Copy link
Contributor

mikkelfj commented Mar 1, 2018

The arguments should be ordered according to the id attribute when present.

In the monster_test.fbs this would be:

ns(Monster_create(pos, mana, hp, name, ...))

because pos has id:0, mana has id: 1, hp has id: 2, and name has id: 3.

but is currently sorted according to the literal text order in the schema:

ns(Monster_create(pos, hp, mana, name, ...))

This is not compatible with Googles C++ create call and can break code if the schema is later rearranged.

In any case it is not possible to add new fields to the schema without a compile error because the create call is not vararg, but this is still safe, unlike the ordering.

This is not trivial to fix because it might break existing code.
Suggestions are welcome.

NOTE: this also potentially relates to JSON parsing because argument vectors might be supported so a Monster table could be given as [pos, hp, mana, name, ...] as opposed to { "pos":pos, "hp":hp, "mana":mana, "name":name}. Since this feature is not currently avaialable it cannot break anything, but if added, it would be inconsistent with the current _create call.

@mikkelfj
Copy link
Contributor Author

mikkelfj commented Mar 1, 2018

There is also the issue with deprecated fields. C++ does have an argument for them, but that can lead to unexpected results as well. FlatCC also currently ignores them.

It could be required that a deprecated argument should always be a zero integer and debug assert if it is anything else.

@mikkelfj mikkelfj added the bug label Mar 14, 2018
@mikkelfj
Copy link
Contributor Author

mikkelfj commented Sep 1, 2018

This is now fixed as a breaking change in v0.5.3.

Deprecated fields are not included in create calls but the arguments are now ordered by field id. For schemas without id attributes there is no difference.

Consider the start of the monster_test.fbs Monster table:

table Monster {
  pos:Vec3 (id: 0);
  hp:short = 100 (id: 2);
  mana:short = 150 (id: 1);
  name:string (id: 3, required, key);
  color:Color = Blue (id: 6);
  inventory:[ubyte] (id: 5);
  friendly:bool = false (deprecated, priority: 1, id: 4);
  /// an example documentation comment: this will end up in the generated code
  /// multiline too
  testarrayoftables:[Monster] (id: 11);
...

The call args in monster_test_builder.h are now:

#define __MyGame_Example_Monster_call_args ,\
  v0, v1, v2, v3,\
  v5, v6, v8, v9,\
  v10, v11, v12, v13,\
  v14, v15, v16, v17,\
  v18, v19, v20, v21,\
  v22, v23, v24, v25,\
  v26, v27, v28, v29,\
  v30, v31, v32, v33, v34, v35

but they used to be

#define __MyGame_Example_Monster_call_args ,\
  v0, v2, v1, v3,\
  v6, v5, v11, v10,\
  v28, v24, v29, v12,\
  v8, v9, v31, v13,\
  v14, v15, v16, v17,\
  v18, v19, v20, v21,\
  v22, v23, v25, v26,\
  v27, v30, v32, v33, v34, v35

Note how v5 now comes before v6.

Also notice how v4 is omitted because it is deprecated and v7 is missing because it is a union type - this hasn't changed after this fix.

@mikkelfj mikkelfj closed this as completed Sep 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant