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 gumbo memory leak, clean up memcheck suppressions, clean up rubocop todos #2830

Merged
merged 5 commits into from
Mar 10, 2023

Conversation

flavorjones
Copy link
Member

What problem is this PR intended to solve?

Some general cleanup

  • fixing a gumbo memory leak found by ruby_memcheck (cc @stevecheckoway)
  • tidying up and documenting some of the remaining suppressions
  • addressing some of the few remaining rubocop todos

Have you included adequate test coverage?

All of this is covered by CI currently.

Does this change affect the behavior of either the C or the Java implementations?

N/A

- fragment initialize doesn't call super (intentionally)
- exclude test and scripts from Metrics/BlockNesting
- exclude test from CollectionLiteralInLoop
- the SAX doc start_element_namespace has a lot of optional args
@flavorjones flavorjones changed the title fix gumbo memory leak, clean up memcheck suppressions, clean up rubocop TODOs fix gumbo memory leak, clean up memcheck suppressions, clean up rubocop todos Mar 10, 2023
@flavorjones
Copy link
Member Author

@stevecheckoway Can you please eyeball the second commit, 4bf4b51

Copy link
Contributor

@stevecheckoway stevecheckoway left a comment

Choose a reason for hiding this comment

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

LGTM.

Good catch on the ignored token.

@flavorjones flavorjones merged commit afa7395 into main Mar 10, 2023
@flavorjones flavorjones deleted the flavorjones-gumbo-memcheck-errors branch March 10, 2023 17:10
@flavorjones flavorjones added this to the v1.15.0 milestone Apr 8, 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.

2 participants