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

fix removeU; add test #1

Closed
wants to merge 2 commits into from
Closed

fix removeU; add test #1

wants to merge 2 commits into from

Conversation

starius
Copy link
Collaborator

@starius starius commented Apr 19, 2015

Previous implementation used to throw the following error:

  binaryheap.lua:99 attempt to compare nil with number

Previous implementation used to throw the following error:

  binaryheap.lua:99 attempt to compare nil with number
@Tieske
Copy link
Owner

Tieske commented Apr 19, 2015

Thanks for the fix.

I don't know whether I like the metatable... It does not resolve the name collision, as inserting a value by a key named after a method, the key will shadow the one in the metatable, so it becomes inaccessible. Also by adding the metatable, there are 2 table lookups to find a method, first on the heap table, second on the metatable.

@starius
Copy link
Collaborator Author

starius commented Apr 19, 2015

Agree, current code state does not solve the problem of method names collision.

I think, the problem with name collisions can be solved by splitting the table into several tables:

  1. head itself (array)
  2. payloads (array)
  3. map from payload to index in head
  4. metatable (for methods)
  5. object, in which first 3 tables are fields and metatable is metatable

2 and 3 are optional (only if payloads are used, i.e. in unique heap). 5 can be eliminated using upvalues instead of fields of a table.

I think, storing each element of a head as a table with fields "value" and "payload" is not as efficient, as two arrays are.

Side note: current implementation requires payloads not to collide with values, because all of them are stored in one table.

@Tieske Tieske closed this in a29e7a3 Apr 19, 2015
@Tieske
Copy link
Owner

Tieske commented Apr 19, 2015

For now, I included the fix, but left the remainder as is. Also just addd a bunch of tests. So I'm confident it works as it should now.

@Tieske
Copy link
Owner

Tieske commented Apr 19, 2015

Side note: current implementation requires payloads not to collide with values, because all of them are stored in one table.

That is not correct, the keys used are array index, and by payload, not by value.

I think, storing each element of a head as a table with fields "value" and "payload" is not as efficient, as two arrays are.

You're probably right. On the wishlist... #2

@starius
Copy link
Collaborator Author

starius commented Apr 19, 2015

Side note: current implementation requires payloads not to collide with values, because all of them are stored in one table.

That is not correct, the keys used are array index, and by payload, not by value.

You are right, values do not collide with payloads, but array indices do. So, numbers can't be used as payloads, as a consequence. I hope this limitation can be removed by #2

@Tieske
Copy link
Owner

Tieske commented Apr 20, 2015

done!

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

Successfully merging this pull request may close these issues.

2 participants