Skip to content

reach: binary_heap bug fixes#42

Open
abagnall-riverside wants to merge 3 commits intomainfrom
reach-binary-heap-fix
Open

reach: binary_heap bug fixes#42
abagnall-riverside wants to merge 3 commits intomainfrom
reach-binary-heap-fix

Conversation

@abagnall-riverside
Copy link
Copy Markdown
Contributor

  • Avoid UB in extract on single-element heap.
  • Use correct index arithmetic for parent and children nodes in _heapify_up and _heapify_down.

- Avoid UB in `extract` on single-element heap.
- Use correct index arithmetic for parent and children nodes in
`_heapify_up` and `_heapify_down`.
.flag();
program.add_argument("-ds", "--dlsym-log")
.help("path to file containing dlsym log of loaded symbols");
program.add_argument("-o", "--output")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this argument removed? Fairly certain it is still used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still there. This removed the second redundant call to add_argument for the --output argument.

void _heapify_down(size_t i) {
size_t left_i = 2 * i;
size_t right_i = 2 * i + 1;
size_t left_i = 2 * i + 1;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment here to explain the indexing structure? I'm fond of ascii art so a graph like this makes sense to me, but choose what you feel is best.

       0
   1       2
 3   4   5    6

Is it supposed to be index i is at i and its children are at 2i +1, 2i+2? I guess this is the result of 0-indexing instead of 1-indexing as the old version would have worked before.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to the top of the file. Honestly, I would rather avoid using a custom binary heap implementation. I just threw this in because I had it lying around.

@srosefield-riverside
Copy link
Copy Markdown
Collaborator

For future-us reference, might want to just replace this implementation with https://en.cppreference.com/w/cpp/algorithm/make_heap.html

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.

3 participants