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

Removed function pointers from heap implementation #482

Merged
merged 3 commits into from
Jul 22, 2024

Conversation

esmakokten
Copy link
Contributor

Summary of this Pull Request (PR)

In order to do stack analysis for components, function pointers are removed from heap impl.
Added macro definition 'DECLARE_HEAP' to define heap with given update and compare functions.
Added unit tests to test heap functionality.

Intent for your PR

Choose one (Mandatory):

  • This PR is for a code-review and is intended to get feedback, but not to be pulled yet.
  • This PR is mature, and ready to be integrated into the repo.

Reviewers (Mandatory):

@gparmer
@mbai1010
@spadek67424

Code Quality

As part of this pull request, I've considered the following:

Style:

  • Comments adhere to the Style Guide (SG)
  • Spacing adhere's to the SG
  • Naming adhere's to the SG
  • All other aspects of the SG are adhered to, or exceptions are justified in this pull request
  • I have run the auto formatter on my code before submitting this PR (see doc/auto_formatter.md for instructions)

Code Craftsmanship:

  • I've made an attempt to remove all redundant code
  • I've considered ways in which my changes might impact existing code, and cleaned it up
  • I've formatted the code in an effort to make it easier to read (proper error handling, function use, etc...)
  • I've commented appropriately where code is tricky
  • I agree that there is no "throw-away" code, and that code in this PR is of high quality

Testing

I've tested the code using the following test programs (provide list here):

  • micro_booter
  • unit_pingpong
  • sched_ping_pong
  • unit_heap
  • unit_schedtests
  • ...(add others here)

@spadek67424
Copy link

spadek67424 commented Jul 13, 2024

Hi @esmakokten @gparmer @mbai1010 ,
From my point, it is good to me. It fixs the function pointer of heap for me.
I do not see the function pointer of heap anymore.

I am not sure about the file structure and coding style. Maybe somebody could have a look?

But I have an off-topic question is that do you miss
a commit "7a0fe09d614f5ec7159fc2ecb72aec380172266e" from your branch?

Maybe you squash the commit?
I do not use PR a lot. Just want to make sure.

@esmakokten
Copy link
Contributor Author

Hi @esmakokten @gparmer @mbai1010 , From my point, it is good to me. It fixs the function pointer of heap for me. I do not see the function pointer of heap anymore.

I am not sure about the file structure and coding style. Maybe somebody could have a look?

But I have an off-topic question is that do you miss a commit "7a0fe09d614f5ec7159fc2ecb72aec380172266e" from your branch?

Maybe you squash the commit? I do not use PR a lot. Just want to make sure.

Hi Ming-Hsien, thanks for the review. Hmm, I have two commits that will be integrated to main branch one of them is the "7a0fe09", I checked the PR and commits it seems correct to me, thanks!

@esmakokten esmakokten closed this Jul 16, 2024
@esmakokten esmakokten reopened this Jul 16, 2024
Copy link
Collaborator

@gparmer gparmer left a comment

Choose a reason for hiding this comment

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

Thanks for this! I think that there are a few annoying style and debug things here, so sorry!!!

src/components/lib/util/heap.h Outdated Show resolved Hide resolved
@gparmer
Copy link
Collaborator

gparmer commented Jul 22, 2024

Thanks a ton! Looks great.

@gparmer gparmer merged commit 99a8118 into gwsystems:main Jul 22, 2024
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.

None yet

3 participants