Skip to content

Arrays of Enumerations without a value for 0 are no longer valid #8836

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.

Sign up for GitHub

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

Jump to bottom
Merged
jtdavis777 merged 5 commits into from
Dec 13, 2025
Merged

Arrays of Enumerations without a value for 0 are no longer valid #8836

jtdavis777 merged 5 commits into from
Dec 13, 2025

Conversation

@jtdavis777
Copy link
Collaborator

@jtdavis777 jtdavis777 commented Dec 8, 2025

Fixes #8833

@github-actions github-actions bot added c++ codegen Involving generating code from schema labels Dec 8, 2025
@jtdavis777 jtdavis777 requested a review from aardappel December 8, 2025 01:03
@aardappel
Copy link
Collaborator

aardappel commented Dec 8, 2025

I have no idea what the changes in the bfbs files etc are about, but this generally LGTM!

@jtdavis777
Copy link
Collaborator Author

jtdavis777 commented Dec 10, 2025

I want to dig a little more into at least one of the bfbs changes just to see what's going on, then will merge if nothing looks improper.

@jtdavis777
Copy link
Collaborator Author

jtdavis777 commented Dec 13, 2025

just dumping my findings here,

the old monster_tests.afb has the follow two extra vtables. the rest of the vtables match up.

vtable (reflection.Field):
  +0x33A6 | 1E 00                   | uint16_t   | 0x001E (30)                        | size of this vtable
  +0x33A8 | 14 00                   | uint16_t   | 0x0014 (20)                        | size of referring table
  +0x33AA | 0C 00                   | VOffset16  | 0x000C (12)                        | offset to field `name` (id: 0)
  +0x33AC | 10 00                   | VOffset16  | 0x0010 (16)                        | offset to field `type` (id: 1)
  +0x33AE | 06 00                   | VOffset16  | 0x0006 (6)                         | offset to field `id` (id: 2)
  +0x33B0 | 08 00                   | VOffset16  | 0x0008 (8)                         | offset to field `offset` (id: 3)
  +0x33B2 | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `default_integer` (id: 4) <defaults to 0> (Long)
  +0x33B4 | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `default_real` (id: 5) <defaults to 0.000000> (Double)
  +0x33B6 | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `deprecated` (id: 6) <defaults to 0> (Bool)
  +0x33B8 | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `required` (id: 7) <defaults to 0> (Bool)
  +0x33BA | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `key` (id: 8) <defaults to 0> (Bool)
  +0x33BC | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `attributes` (id: 9) <null> (Vector)
  +0x33BE | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `documentation` (id: 10) <null> (Vector)
  +0x33C0 | 05 00                   | VOffset16  | 0x0005 (5)                         | offset to field `optional` (id: 11)
  +0x33C2 | 0A 00                   | VOffset16  | 0x000A (10)                        | offset to field `padding` (id: 12)

vtable (reflection.Field):
  +0x31F8 | 1C 00                   | uint16_t   | 0x001C (28)                        | size of this vtable
  +0x31FA | 10 00                   | uint16_t   | 0x0010 (16)                        | size of referring table
  +0x31FC | 08 00                   | VOffset16  | 0x0008 (8)                         | offset to field `name` (id: 0)
  +0x31FE | 0C 00                   | VOffset16  | 0x000C (12)                        | offset to field `type` (id: 1)
  +0x3200 | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `id` (id: 2) <defaults to 0> (UShort)
  +0x3202 | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `offset` (id: 3) <defaults to 0> (UShort)
  +0x3204 | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `default_integer` (id: 4) <defaults to 0> (Long)
  +0x3206 | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `default_real` (id: 5) <defaults to 0.000000> (Double)
  +0x3208 | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `deprecated` (id: 6) <defaults to 0> (Bool)
  +0x320A | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `required` (id: 7) <defaults to 0> (Bool)
  +0x320C | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `key` (id: 8) <defaults to 0> (Bool)
  +0x320E | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `attributes` (id: 9) <null> (Vector)
  +0x3210 | 00 00                   | VOffset16  | 0x0000 (0)                         | offset to field `documentation` (id: 10) <null> (Vector)
  +0x3212 | 07 00                   | VOffset16  | 0x0007 (7)                         | offset to field `optional` (id: 11)

@jtdavis777
Copy link
Collaborator Author

jtdavis777 commented Dec 13, 2025 •
edited
Loading

the first vtable is pointed to by

table (reflection.Field):
  +0x33C4 | 1E 00 00 00             | SOffset32  | 0x0000001E (30) Loc: 0x33A6        | offset to vtable
  +0x33C8 | 00                      | uint8_t[1] | .                                  | padding
  +0x33C9 | 01                      | uint8_t    | 0x01 (1)                           | table field `optional` (Bool)
  +0x33CA | 05 00                   | uint16_t   | 0x0005 (5)                         | table field `id` (UShort)
  +0x33CC | 1A 00                   | uint16_t   | 0x001A (26)                        | table field `offset` (UShort)
  +0x33CE | 02 00                   | uint16_t   | 0x0002 (2)                         | table field `padding` (UShort)
  +0x33D0 | 18 00 00 00             | UOffset32  | 0x00000018 (24) Loc: 0x33E8        | offset to field `name` (string)
  +0x33D4 | 04 00 00 00             | UOffset32  | 0x00000004 (4) Loc: 0x33D8         | offset to field `type` (table)

the second is referred to by a pair of tables

table (reflection.Field):
  +0x310C | 14 FF FF FF             | SOffset32  | 0xFFFFFF14 (-236) Loc: 0x31F8      | offset to vtable
  +0x3110 | 00 00 00                | uint8_t[3] | ...                                | padding
  +0x3113 | 01                      | uint8_t    | 0x01 (1)                           | table field `optional` (Bool)
  +0x3114 | 18 00 00 00             | UOffset32  | 0x00000018 (24) Loc: 0x312C        | offset to field `name` (string)
  +0x3118 | 04 00 00 00             | UOffset32  | 0x00000004 (4) Loc: 0x311C         | offset to field `type` (table)

table (reflection.Field):
  +0x3214 | 1C 00 00 00             | SOffset32  | 0x0000001C (28) Loc: 0x31F8        | offset to vtable
  +0x3218 | 00 00 00                | uint8_t[3] | ...                                | padding
  +0x321B | 01                      | uint8_t    | 0x01 (1)                           | table field `optional` (Bool)
  +0x321C | 18 00 00 00             | UOffset32  | 0x00000018 (24) Loc: 0x3234        | offset to field `name` (string)
  +0x3220 | 04 00 00 00             | UOffset32  | 0x00000004 (4) Loc: 0x3224         | offset to field `type` (table)

@jtdavis777
Copy link
Collaborator Author

jtdavis777 commented Dec 13, 2025

alright -- the arrays_test.bfbs was a little easier to take apart to see changes.
image

old:

table (reflection.Field):
  +0x029C | 50 FE FF FF             | SOffset32  | 0xFFFFFE50 (-432) Loc: 0x044C  | offset to vtable
  +0x02A0 | 00 00 00                | uint8_t[3] | ...                            | padding
  +0x02A3 | 01                      | uint8_t    | 0x01 (1)                       | table field `optional` (Bool)
  +0x02A4 | 05 00                   | uint16_t   | 0x0005 (5)                     | table field `id` (UShort)
  +0x02A6 | 90 00                   | uint16_t   | 0x0090 (144)                   | table field `offset` (UShort)
  +0x02A8 | 14 00 00 00             | UOffset32  | 0x00000014 (20) Loc: 0x02BC    | offset to field `name` (string)
  +0x02AC | 04 00 00 00             | UOffset32  | 0x00000004 (4) Loc: 0x02B0     | offset to field `type` (table)

new:

table (reflection.Field):
  +0x029C | 04 FE FF FF             | SOffset32  | 0xFFFFFE04 (-508) Loc: 0x0498  | offset to vtable
  +0x02A0 | 05 00                   | uint16_t   | 0x0005 (5)                     | table field `id` (UShort)
  +0x02A2 | 90 00                   | uint16_t   | 0x0090 (144)                   | table field `offset` (UShort)
  +0x02A4 | 14 00 00 00             | UOffset32  | 0x00000014 (20) Loc: 0x02B8    | offset to field `name` (string)
  +0x02A8 | 04 00 00 00             | UOffset32  | 0x00000004 (4) Loc: 0x02AC     | offset to field `type` (table)

seems the logic around the optional keyword was affected.

@jtdavis777
Copy link
Collaborator Author

jtdavis777 commented Dec 13, 2025

okay I figured it out! safe to merge. old code was explicitly setting the optional field on struct and array members of structs -- which has no effect and is an error.

@jtdavis777 jtdavis777 merged commit c9a301e into google:master Dec 13, 2025
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aardappel aardappel aardappel approved these changes

Assignees

No one assigned

Labels

c++ codegen Involving generating code from schema

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Inconsistent handling of enumerations

2 participants

@jtdavis777 @aardappel