search

vermaseren / form

The FORM project for symbolic manipulation of very big expressions
GNU General Public License v3.0
1.15k stars 136 forks source link

Fix memory leak in makeinteger #509

Closed jodavies closed 4 months ago

jodavies commented 4 months ago

We need to call NumberFree each loop iteration, not only upon leaving execarg.

This fixes #508 but I think it needs more testing.

coveralls commented 4 months ago

Coverage Status

coverage: 48.735% (+0.02%) from 48.713% when pulling 5b204817aa590dc7e551b727f978e18700a79a8d on jodavies:issue-508 into b82093350ae986ad6c65a9ad25bf748efa53d7be on vermaseren:master.

jodavies commented 4 months ago

Adding a test to the CI for this one is a bit tricky, because the number of terms required for the crash is system dependent.

jodavies commented 4 months ago

This most recent commit seems like the best way to do this, but I don't see a performance difference compared to the commit before. I think NumberMalloc calls are just really cheap, and as long as you are not leaking and extending the buffer all of the time, it doesn't matter much how many you allocate.

tueda commented 4 months ago

You can rebase this PR like this: tueda/jodavies_issue-508_rebased (then git diff with 81862dc2567f2fd52b6c5da5cac59ab1e7f0b4c8 shows nothing).

jodavies commented 4 months ago

Should I not just flatten all commits for a cleaner merge?

tueda commented 4 months ago

Right, in the end, squashing all to one or two commits is good. (I thought first rebasing is needed, but it seems that you can squash in one go.)

tueda commented 4 months ago

Now this seems fine. I will revisit it tomorrow morning and probably will merge it.