200sc / bebop

bebop wire format in Go
Apache License 2.0
71 stars 5 forks source link

bug: tmp variable redeclared in generated code #44

Closed mirza-s closed 7 months ago

mirza-s commented 7 months ago

Hey :wave:, if we try to generate code from the following definitions:

struct Fruit {
        uint64 ID;
        string Name;
}

struct Basket {
        Fruit Apples;
        Fruit Oranges;
        Fruit Mangos;
}

we get the following:

        tmp := (bbp.Apples)
        at += tmp.Size()
        bbp.Oranges, err = MakeFruitFromBytes(buf[at:])
        if err != nil {
                return err
        }
        tmp := (bbp.Oranges)
        at += tmp.Size()

where tmp is redeclared each time a struct is referenced.

I think I can fix this one either by checking that tmp is declared only once or by encapsulating tmp usage inside a { block } which does not require additional state keeping but produces uglier code.

Where would be a nice place to put a unit test for this?

Cheers, Mirza

200sc commented 7 months ago

That's a fairly basic input that I'm surprised isn't covered by existing tests; what version or commit hash are you on, to verify? I have time to patch this, I suspect.

mirza-s commented 7 months ago

This was tested with bebopc-go v0.5.0 and I've retested on d6bc82fc7ccc52f292ea999e6b9420e350f6c71e

200sc commented 7 months ago

https://github.com/200sc/bebop/pull/45 adds blocking around the variables; I'm open to a prettier solution, i.e. tracking if tmp needs a := or = per function, but that might require a more significant overhaul of code generation to add some state tracking to a few functions.

200sc commented 7 months ago

@mirza-s If you'd like to take a stab at outputting some nicer code, and are happy to wait (or use 0.4.0 instead which shouldn't present this problem), I can hold off on merging that branch; what do you think?

mirza-s commented 7 months ago

Thanks for the quick fix. The current solution is fine by me :+1: