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

The force_align attribute is allowed on enums, but does nothing [all languages] #7515

Closed
TethysSvensson opened this issue Sep 9, 2022 · 5 comments · Fixed by #7523
Closed

Comments

@TethysSvensson
Copy link
Contributor

This schema is accepted by flatc:

enum Foo: uint8 (force_align: 4) { X, Y, Z }

However it generates the same code as the following schema:

enum Foo: uint8 { X, Y, Z }

I think this should work the same as force_align on structs. Alternatively it could be rejected by flatc.

I have validated that the bug is present in versions 2.0.0, 2.0.7 and the current master.

@dbaileychess
Copy link
Collaborator

May I ask why you would want to force align an enum to something other than the backing type alignment?

I'm leaning to rejecting if by flatc instead of allowing it.

@TethysSvensson
Copy link
Contributor Author

I don't think I see a good reason for it.

However I am not a good authority on whether it's useful, since I don't think having alignment in the language is something that is beneficial.

@dbaileychess
Copy link
Collaborator

Sure, then why are you adding the force_align attribute?

Thanks for the report, I'll see about forbidding from being applied to enums.

@TethysSvensson
Copy link
Contributor Author

We are aiming for planus to have as much feature parity with flatc as possible.

Also, since that flatbuffers does have alignment built-in to the language, using force_align is a good way for me to test the correctness of the alignment code in planus.

@dbaileychess
Copy link
Collaborator

SG. Since adding the attribute seems to have no negative effect, this will have slightly lower priority to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants