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

Lua Generator using IR. #6940

Merged
merged 38 commits into from
Dec 3, 2021
Merged

Lua Generator using IR. #6940

merged 38 commits into from
Dec 3, 2021

Conversation

dbaileychess
Copy link
Collaborator

@dbaileychess dbaileychess commented Nov 19, 2021

Address: #6926

This implements the first generator using the the reflection.fbs schema as an IR between the flatc parser and code gen. It should serve as a model of how to write other generators using this approach.

I tried to keep things as well separated as possible, where this new generator only relies on the public flatbuffer.h headers and generated reflection_generated.h and knows nothing about the internals of the flatc parser. This required modifying the reflection.fbs a bit to provide some missing information. The tests/monster_test_bfbs_generated.h shows only a minor size increase.

The main interface for generation is the BfbsGenerator interface that has a single method:

GeneratorStatus generate(const uint8_t *buffer, int64_t length) = 0;

This simulates some random process reading in the IR buffer from somewhere (e.g., file, disk, memory). To avoid needing a separate binary, I integrated this new generator directly in the flatc binary, and pass the IR as a in-mem struct.

I kept the API as simple as possible, so that the generators can be implemented in whatever fashion they want, as long as they use this BfbsGenerator interface. For this Lua generator, I did make a BaseBfbsGenerator class that fulfills the BfbsGenerator interface and has some utility methods that should be applicable to any language generator.

This generator replaces the one for --lua, and I'll remove the old idl based on in a follow up CL.

I did modify the output of the generated Lua files a bit, so that is why there is a large diff between the two. But ignoring white space and some name simplifications, the core logic remains the identical.

I ran tests/LuaTest.sh with all the supported Lua flavors and they all passed:

[luajit]
# of test passed: 7 / 7 (100.00%)
[lua5.1]
# of test passed: 7 / 7 (100.00%)
[lua5.2]
# of test passed: 7 / 7 (100.00%)
[lua5.3]
# of test passed: 7 / 7 (100.00%)
[lua5.4]
# of test passed: 7 / 7 (100.00%)

@github-actions github-actions bot added c++ codegen Involving generating code from schema lua python labels Nov 19, 2021
Copy link
Collaborator

@aardappel aardappel left a comment

Choose a reason for hiding this comment

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

some quick initial thoughts

reflection/reflection.fbs Outdated Show resolved Hide resolved
reflection/reflection.fbs Outdated Show resolved Hide resolved
src/base_generator.h Outdated Show resolved Hide resolved
src/idl_parser.cpp Outdated Show resolved Hide resolved
src/lua_generator.cpp Outdated Show resolved Hide resolved

append_comments(object_def->documentation());
append_line("local " + object_name + " = {}");
append_line("local mt = {}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to be very much a preference thing, but this feels like a very verbose style of codegen to me. That would be ok if it was just for Lua, but by putting these functions in the base class you're implying that all such generators should use this style.

What should we use instead? Frankly, if its just down to me, I would not have such functions at all, and just += raw strings, including embedded indentation spaces. I think all this procedural indenting is overabstraction for any language that has a clear code style. Even for C++ we just output Google Style code, so it doesn't matter. And not having all these function calls in between makes it all much easier to read.

If that is too basic for you, maybe something like what the C++ generator does, with overloaded += etc.. but not a fan of that either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noted. I'll moved out of base class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do prefer the more verbose style, especially if performance is not an issue. Which it isn't during code gen. The compiler is just going to inline all the method calls anyways.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll leave it for now and we can refactor after this is in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

While we're bike shedding, my 2c is that I like the += thing. I also try to use methods like ForAllFieldsInTable or ForAllVariantsInEnum which take a lambda and handles indentation. This is neat since the code in the lambda will be indented anyway. If the ForAllX method did not handle indentation, then the raw string would seem double indented. This is of course pretty far on the DRY/overengineered side of the spectrum

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you suggesting placing ForAllFieldsInTable and ForAllVariantsInEnum in the base class and then the implementing classes just provide a custom lambda?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went ahead and refactored it all to use the code += style of code gen.

I took @CasperN idea to define those functions that took a lambda, as that hid away the details of how we were looping over the vectors.

@dbaileychess
Copy link
Collaborator Author

I ended up replacing the old --lua generator with this new one by default. I'll remove the old code in a follow up CL so it is easier to revert if needed.

src/bfbs_gen.h Outdated Show resolved Hide resolved
src/bfbs_gen.h Outdated Show resolved Hide resolved
@dbaileychess dbaileychess merged commit 061d61f into google:master Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ CI Continuous Integration codegen Involving generating code from schema lua python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants