travisstaloch / flatbufferz Goto Github PK

from #2 (comment) by @clickingbuttons

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);
    }
}

3. Object API optionals should be optional_field: ?T instead of optional_field: ?*T to prevent unnecessary allocation.
4. 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.

The suffix T is a misnomer.

Zig has a style guide, but it's subject to change. Everyone has their own preferences coming from different languages.

Save a dereferencing .? and a memory allocation per-field. Change to optional_field: ?T instead of optional_field: ?*T.

Fbs:

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);
    }
}

generate simple tests which do a serialize/deserialize/serialize cycle and compare the resulting bytes

this makes running scripts/test-all.sh useful for catching compile errors in generated code

Arrays are only allowed inside of structs and are laid out like sequential struct fields.

struct Vec4 {
  v: [float:4];
}

Gives:

thread 280733 panic: TODO TypeFmt.init() base_ty=reflection.BaseType.fb.BaseType.Array
/home/thesm/.zvm/master/lib/std/debug.zig:291:22: 0x30b555 in panicExtra__anon_19121 (flatc-zig)
    std.builtin.panic(msg, trace, ret_addr);
                     ^
/home/thesm/.zvm/master/lib/std/debug.zig:266:15: 0x2e9449 in panic__anon_17084 (flatc-zig)
    panicExtra(null, null, format, args);
              ^
/home/thesm/Downloads/flatbufferz/src/common.zig:19:20: 0x2c9318 in todo__anon_16220 (flatc-zig)
    std.debug.panic("TODO " ++ fmt, args);
                   ^
/home/thesm/Downloads/flatbufferz/src/codegen.zig:183:20: 0x2c8f54 in format__anon_16215 (flatc-zig)
        } else todo("TypeFmt.init() base_ty={}", .{base_ty});

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

--no-gen-object-api still imports object API ...T types and Unpack functions taking a ...T.

Ideally this flag is just removed and always turned on since it allows for nicer Unpack methods.

More convenient. Requires object API.

Useful for when Zig breaks codegenned syntax and for codegen devs. Here's one way to do it:

fn format(allocator: Allocator, fname: []const u8, code: [:0]const u8) ![]const u8 {
    var ast = try std.zig.Ast.parse(allocator, code, .zig);
    defer ast.deinit(allocator);

    if (ast.errors.len > 0) {
        for (ast.errors) |err| {
            var buf = std.ArrayList(u8).init(allocator);
            defer buf.deinit();
            ast.renderError(err, buf.writer()) catch {};
            log.err("formatting {s}: {s}", .{ fname, buf.items });
        }
        return code;
    }

    return try ast.render(allocator);
}

For the following FBS:

enum AdvancedFeatures : ulong (bit_flags) {
    AdvancedArrayFeatures,
    AdvancedUnionFeatures,
    OptionalScalars,
    DefaultVectorsAndStrings,
}

The following enum is generated:

pub const AdvancedFeatures = enum(u64) {
    AdvancedArrayFeatures = 1,
    AdvancedUnionFeatures = 2,
    OptionalScalars = 4,
    DefaultVectorsAndStrings = 8,
    pub fn tagName(v: @This()) []const u8 {
        return @tagName(v);
    }
};

Which causes the @intToEnum cast to fail if multiple bit flags (or none) are set:

    pub fn AdvancedFeatures(rcv: Schema) reflection.AdvancedFeatures {
        const o = rcv._tab.offset(16);
        if (o != 0) {
            return rcv._tab.read(reflection.AdvancedFeatures, o + rcv._tab.pos);
        }
        return @intToEnum(reflection.AdvancedFeatures, 0);
    }

One potential fix is to generate a packed struct instead of an enum and read it like you would any other scalar field. The packed struct looks like this:

pub const AdvancedFeatures = packed struct {
    AdvancedArrayFeatures: bool,
    AdvancedUnionFeatures: bool,
    OptionalScalars: bool,
    DefaultVectorsAndStrings: bool,
};

Make an identifier string pool per-file to handle naming clashes. Give user fields and enums their function, field, and type identifiers. Then:

1. Give modules their identifiers
2. Give inner fields (like _tab: flatbufferz.Table) their identifiers
3. Give function parameters their identifiers
4. Give temporary variables their identifiers

This will allow removing _s from field names (like _tab) and temporaries. It will also allow consistently naming the first self: Self parameter.