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

Evaluate and implement a consistent and safe style for temporary variables. #15

Open
Lusito opened this issue Nov 9, 2020 · 0 comments
Labels
Cleanup help wanted Extra attention is needed Refactoring

Comments

@Lusito
Copy link
Owner

Lusito commented Nov 9, 2020

JavaScript has a garbage collector.. we gotta live with it.

In the original code, temporary vectors and similar where created on the stack without problems.
If we use new objects everytime, we'll just push work for the garbage collector.

Flyover has 2 solutions for this:

  • Making functions take an extra out-parameter
  • Adding static variables to the classes and reusing these variables when doing internal computations.

The former is fine.

The latter, however, is a but ugly to work with and compare code. This also allows to access these static variables from other classes, which flyover does a lot with the b2Vec2 static attributes. This requires you to have a good eye on these to avoid accidental double use.

In order to make the code easier to compare to upstream, I used top-level constants instead in some places, but that approach isn't perfect either:

const temp = {
    J1: new b2Vec2(),
    J2: new b2Vec2(),
    J3: new b2Vec2(),
    r: new b2Vec2(),
    e1: new b2Vec2(),
    e2: new b2Vec2(),
    Jd1: new b2Vec2(),
    Jd2: new b2Vec2(),
    d: new b2Vec2(),
    u: new b2Vec2(),
    dp1: new b2Vec2(),
    dp2: new b2Vec2(),
    dp3: new b2Vec2(),
    d1: new b2Vec2(),
    d2: new b2Vec2(),
    dHat: new b2Vec2(),
};

When I'm talking about double use, I mean something like:

function doStuff(m: b2Vec2, n: b2Vec2, out: b2Vec2) {
    const t = b2Vec2.s_t0.Set(0, 1);
    // ...
    out.x = m.x + n.x + t.x;
    out.y = m.y + n.y + t.y;
    return out;
}

function accidentalDoubleUse() {
    const a = new b2Vec2(1, 2);
    const b = new b2Vec2(2, 3);
    const c = doStuff(a, b2Vec2.Add(a, b, b2Vec2.s_t0), b2Vec2.s_t1);
    return c.y;
}

Both functions use b2Vec2.s_t0, causing loss of data.

The goal of this task would be to evaluate the best approach to encapsulate the temp variables, so they can't easily get used in two places at once.

Then when implementing these changes, ensure, that no public temporary variables get shared over multiple modules.

@Lusito Lusito added Cleanup help wanted Extra attention is needed Refactoring labels Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup help wanted Extra attention is needed Refactoring
Projects
None yet
Development

No branches or pull requests

1 participant