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

Use camelCaseFunctionNames in generated files #2

Closed
thesmartwon opened this issue May 17, 2023 · 3 comments
Closed

Use camelCaseFunctionNames in generated files #2

thesmartwon opened this issue May 17, 2023 · 3 comments

Comments

@thesmartwon
Copy link

Instead of TitleCaseFunctionNames follow https://ziglang.org/documentation/master/#Names and use camelCaseFunctionNames like the language and standard library.

@travisstaloch
Copy link
Owner

I like the idea and would favor using lower camel case names in generated zig code. However, all of the other implementations I looked at used title camel case including go, cpp and python if i remember correctly. So I chose to follow them because its a little easier to compare generated code.

I'm not sure why this is a thing. I can see why the go impl does this as it follows the go style guide. But the cpp and python impls aren't following their style guides.

Maybe we could add a build or config option for changing the function casing? I would be glad to accept a PR for this. But I'd like to default to title case for now, at least until a day when this project is more trusted (known to work well) and comparing generated code shouldn't be necessary.

@clickingbuttons
Copy link

Hey @travisstaloch , would you accept a PR refactoring src/codegen.zig?

Serious issues

  1. --no-gen-object-api still imports object API ...T types and Unpack functions
  2. Property name collisions with unions can lead to generated code calling non-existent functions. For example:
union Type { Null, Int }
table Field { type: Type }

generates a non-existent call to rcv.Type_Type() instead of the correct rcv.TypeType():

pub fn UnpackTo(rcv: Field, t: *FieldT, __pack_opts: fb.common.PackOptions) !void {
    if (rcv.Type_()) |_tab| {
        t.type = try TypeT.Unpack(rcv.Type_Type(), _tab, __pack_opts);
    }
}
  1. Object API optionals should be optional_field: ?T instead of optional_field: ?*T to prevent unnecessary allocation.
  2. Add error printing on invalid codegen (useful for when Zig breaks codegenned syntax and for codegen devs)

Style issues

  1. Remove _s from variable names. Zig purposefully disallows private struct members. Also, constants like __file_indent are already namespaced to the source file. Parameters like __builder can simply be renamed to builder.
  2. Consistently use self: Self instead of inconsitently rcv: {type} and self: {type}
  3. Add option for an index file with all generated types since there's a file generated per table/struct/union which can add up quickly.
  4. As for the casing, I think a reason for Title over snake_case is to prevent parameter name shadowing like:
pub fn rcv(rcv: Field) bool {
    ...
}

This can of course be solved by renaming the parameter to rcv_ or something similar but adds another layer of complexity.

Sorry to hijack this issue, let me know if there's a better way to contact you. I'm happy to make all these changes.

@travisstaloch
Copy link
Owner

Hey @travisstaloch , would you accept a PR refactoring src/codegen.zig?

Sorry so long. Looked at this earler today and forgot to respond.

These all sound like legitimate problems to me and I would gladly accpt a PR. 👍

I've created #3 with the text from above. Lets discuss there

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

No branches or pull requests

3 participants