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

Cosmetic Index out of bound check fix. #123

Closed
martinfrances107 opened this issue Apr 20, 2021 · 1 comment
Closed

Cosmetic Index out of bound check fix. #123

martinfrances107 opened this issue Apr 20, 2021 · 1 comment

Comments

@martinfrances107
Copy link
Contributor

martinfrances107 commented Apr 20, 2021

I porting this test to rust... I found an accidental inconsistency that because of javascript being a bit screwy
well it accidentally does the right thing...

I still consider it bug as someone modifying/maintaining the java script may not see the bug until too late..

this test

tape("delaunay.voronoi() for one point returns the bounding rectangle", test => {
  let voronoi = Delaunay.from([[0, 0]]).voronoi([-1, -1, 2, 2]);
  test.equal(voronoi.renderCell(0), "M2,-1L2,2L-1,2L-1,-1Z");
  test.equal(voronoi.render(), null);
});

When I watch the code below run in a debugger.
(delaunay.js)

    // degenerate case: 1 or 2 (distinct) points
    if (hull.length <= 2 && hull.length > 0) {
      this.triangles = new Int32Array(3).fill(-1);
      this.halfedges = new Int32Array(3).fill(-1);
      this.triangles[0] = hull[0];
      this.triangles[1] = hull[1];   /* index out of bounds */
      this.triangles[2] = hull[1];   /* index out of bounds */
      inedges[hull[0]] = 1;
      if (hull.length === 2) inedges[hull[1]] = 0; /* move statements here */
    }

hull has a length of 1 with the contents of [0];

initially this.triangles is set to [-1, -1, -1]

hull[1] is formally undefined
and because Javascript is screwy

this.triangles[1] = undefined is a noop ... well after this.triangles[1] is still -1

My proposal is to move the statements labelled "index out of bounds" down into the
existing if statements which covers this case...

it will have no effect.. just make the intent of the code clear.
(actually it does less work, but that is not the advantage I am talking about)

I am about to generate a PR.

martinfrances107 added a commit to martinfrances107/d3-delaunay that referenced this issue Apr 20, 2021
@Fil
Copy link
Member

Fil commented Apr 20, 2021

fixed by 54795cb

@Fil Fil closed this as completed Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants