feat. Add valgrind on PR #236
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
remiolivier-discord
force-pushed
the
remi/add-valgrind-on-pr
branch
from
3e827f0 to
c396ea3
Compare
salarkhan
reviewed
Apr 3, 2025
| for test_case in "${test_cases[@]}"; do | ||
| read -r height width input output <<< "$test_case" | ||
| echo "Testing with height=$height width=$width input=$input output=$output" | ||
| VALGRIND_OUT=$(valgrind --leak-check=full ./example \ |
There was a problem hiding this comment.
consider adding --show-leak-kinds=all?
"Definitely lost": Memory that was allocated but never freed
"Indirectly lost": Memory that was allocated but lost due to losing pointers to it
"Possibly lost": Memory that Valgrind can't determine if you have a pointer to
"Still reachable": Memory that wasn't freed but a pointer to it still exists
"Suppressed": Memory leaks that were suppressed via suppression files
There was a problem hiding this comment.
The problem is with go we will always get possibly lost and still reachable because of its GC. So here we will just care about the most important leaks
There was a problem hiding this comment.
yeah, but i figured it might be nice to at least have the output. if possibly lost / still reachable has a huge increase we'd still want to know, no? not a strong opinion here though, agreed that the important ones are already covered
salarkhan
approved these changes
Apr 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add some memory leak checks for lilliput
Testing memory leaks with valgrind, c++ and Go is a little hard but we can add basic checks in
definitely lostandindirectly lostat least for the c++ code we call underneath.We had a few regression in the past showing issues with these 2 and these checks while not extensive will already go a long way.