Skip to content

Remove all memory leaks from vinumc#139

Merged
Grillo-0 merged 7 commits into
developfrom
vinumc-remove-leaks
Jun 6, 2026
Merged

Remove all memory leaks from vinumc#139
Grillo-0 merged 7 commits into
developfrom
vinumc-remove-leaks

Conversation

@Grillo-0

Copy link
Copy Markdown
Member

To test this I configured the meson build folder with:

meson configure build/ -Db_sanitize=address

and ran the current tests. There is still one testing failing with this configuration, but it's the vutils allocator tests and it's only because of how the test are done.

@Grillo-0 Grillo-0 requested review from JeanJPNM and artP2 May 22, 2026 07:31
@Grillo-0 Grillo-0 self-assigned this May 22, 2026

@JeanJPNM JeanJPNM left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While the code works, I'm concerned about the usage of non-growable arena allocators specifically for handling user-generated strings, since they can be arbitrarily large and could easily compound past 10KiB for large documents or when we add support for importing multiple files.

Comment thread subprojects/vinumc/dry_bison.y Outdated
Comment thread subprojects/vinumc/libvinumc.c Outdated
@Grillo-0 Grillo-0 changed the title Remove all memoriy leaks and from vinumc Remove all memory leaks from vinumc May 25, 2026
@Grillo-0

Copy link
Copy Markdown
Member Author

I'm concerned about the usage of non-growable arena allocators specifically for handling user-generated strings, since they can be arbitrarily large and could easily compound past 10KiB for large documents or when we add support for importing multiple files.

Let's create an issue for that to not hang this PR in review

@Grillo-0 Grillo-0 force-pushed the vinumc-remove-leaks branch 2 times, most recently from ee66603 to 4c5380d Compare May 29, 2026 12:28
@Grillo-0 Grillo-0 requested a review from JeanJPNM May 29, 2026 12:33
@artP2

artP2 commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

can you execute the sanize again? And also fix the commit mess I did in resolving the conflicts (only browser access here)

Grillo-0 added 6 commits June 6, 2026 05:04
We need to track the allocations made on the node `.childs` property to
in the end free them. Do that by adding an arena to the AST. To force
it's use, remove all functions that create standalone nodes and provide
functions that creta nodes already linked with the AST.
@Grillo-0 Grillo-0 force-pushed the vinumc-remove-leaks branch from 58aad60 to 191f97c Compare June 6, 2026 08:22
We need to free the child ptrs and areanas in the end of compilation. To
be able to that, add arenas to the evaluation context.
@Grillo-0 Grillo-0 force-pushed the vinumc-remove-leaks branch from 191f97c to e36031f Compare June 6, 2026 08:24

@JeanJPNM JeanJPNM left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@Grillo-0 Grillo-0 merged commit ef99402 into develop Jun 6, 2026
3 checks passed
@Grillo-0 Grillo-0 deleted the vinumc-remove-leaks branch June 6, 2026 21:29
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