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

Add min-size and max-size filter to heap chunks command #1025

Merged
merged 11 commits into from Dec 21, 2023

Conversation

r12f
Copy link
Contributor

@r12f r12f commented Dec 19, 2023

Description

This change adds --min-size and --max-size filter to heap chunks command.

With heap summary, we can quickly use --min-size and --max-size filter to find some sample chunks for further analysis.

Here are the screenshots as demo:
image

image

image

  • My code follows the code style of this project.
  • My change includes a change to the documentation, if required.
  • If my change adds new code, adequate tests have been added.
  • I have read and agree to the CONTRIBUTING document.

@r12f
Copy link
Contributor Author

r12f commented Dec 19, 2023

It looks like CI failed because of pipeline issue, not related to code. Just in case this is somehow considered blocking.

image

@hugsy
Copy link
Owner

hugsy commented Dec 20, 2023

Hi @r12f

It looks like CI failed because of pipeline issue, not related to code. Just in case this is somehow considered blocking.

It is not blocking, we're all just very busy lately. Yes, disregard this failure, I thought that issue was fixed but GHActions continues being annoying af. I will review your PR during the week.

👋

@hugsy hugsy self-assigned this Dec 20, 2023
@r12f
Copy link
Contributor Author

r12f commented Dec 20, 2023

sounds perfect! thanks a lot for confirming @hugsy !

Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

A good PR with just a few minor things to change and we're good !

gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
tests/commands/heap.py Show resolved Hide resolved
@r12f
Copy link
Contributor Author

r12f commented Dec 20, 2023

thanks a lot for reviewing the PR! the comments are addressed now :D

Copy link
Collaborator

@Grazfather Grazfather left a comment

Choose a reason for hiding this comment

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

Looks great. Added a comment about maybe avoiding an if around should process by checking once at the top. Might not make sense logically, I didn't look too closely. If you disagree, that's fine and @hugsy can go ahead and merge with my blessing.

gef.py Show resolved Hide resolved
@Grazfather Grazfather merged commit fbda021 into hugsy:main Dec 21, 2023
4 of 5 checks passed
@r12f
Copy link
Contributor Author

r12f commented Dec 21, 2023

Woot! thanks a lot for the review!

@r12f r12f deleted the user/r12f/size branch December 21, 2023 02:16
@hugsy hugsy added this to the 2024.01 milestone Dec 22, 2023
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