| Message ID | cover.1721995576.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Memory leak fixes (pt.3) | expand |
Patrick Steinhardt <ps@pks.im> writes: > Hi, > > I originally wanted to hold off with sending out this series until v2.46 > was out. But I saw that Junio sent out some patches which are plugging > the same leaks as I did, so I dedcided to send it out now to avoid some > duplicated work. > > There isn't really any structure to this series, I just happened to pick > some random test suites that fail with the leak checker enabled and then > fixed those. Naturally, I've also got part 4 of this series of patch > series in the pipeline already :) As mentioned elsewhere, I hope to get > the number of failing test suites to zero this year. Let's see whether > this is realistic. > > Patrick > This was quite easy to review since there wasn't much to add and it was clearly split into small commits. Apart from some nits, the series looks great to me. Thanks, Karthik
On Tue, Jul 30, 2024 at 07:09:57AM -0400, Karthik Nayak wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > Hi, > > > > I originally wanted to hold off with sending out this series until v2.46 > > was out. But I saw that Junio sent out some patches which are plugging > > the same leaks as I did, so I dedcided to send it out now to avoid some > > duplicated work. > > > > There isn't really any structure to this series, I just happened to pick > > some random test suites that fail with the leak checker enabled and then > > fixed those. Naturally, I've also got part 4 of this series of patch > > series in the pipeline already :) As mentioned elsewhere, I hope to get > > the number of failing test suites to zero this year. Let's see whether > > this is realistic. > > > > Patrick > > > > This was quite easy to review since there wasn't much to add and it was > clearly split into small commits. Apart from some nits, the series looks > great to me. Thanks for your review! Given that until now the only thing that needs fixing is smallish typoes I'll refrain from sending the whole thing out just to fix those. I've fixed them locally though and will send them out in case anything else needs fixing. Patrick
On Fri, Jul 26, 2024 at 02:13:43PM +0200, Patrick Steinhardt wrote:
> 57 files changed, 251 insertions(+), 73 deletions(-)
I took a careful read through these patches, and found most of them easy
to review. I was admittedly a little lost with the "fix various leak"
patches, and having slightly more context in the commit descriptions
there would have been helpful.
I think most of these patches look all good to me, but there is one
where I think using a strvec would be a better fit than allocating our
own array.
Otherwise, these are looking in good shape. Thanks for working on this!
Thanks,
Taylor
On Wed, Jul 31, 2024 at 01:01:07PM -0400, Taylor Blau wrote: > On Fri, Jul 26, 2024 at 02:13:43PM +0200, Patrick Steinhardt wrote: > > 57 files changed, 251 insertions(+), 73 deletions(-) > > I took a careful read through these patches, and found most of them easy > to review. I was admittedly a little lost with the "fix various leak" > patches, and having slightly more context in the commit descriptions > there would have been helpful. Yeah, I was a bit torn here on whether to expand the commit messages or not. I just didn't think it's all that useful to always say "Variable x is allocated, but never freed" for trivial cases where allocation and freeing need to happen in the same function, much less so if we repeat such commits for every single such variable in the same subsystem. So I just threw those fixes into a single bag. Patrick
On Thu, Aug 01, 2024 at 10:19:45AM +0200, Patrick Steinhardt wrote: > On Wed, Jul 31, 2024 at 01:01:07PM -0400, Taylor Blau wrote: > > On Fri, Jul 26, 2024 at 02:13:43PM +0200, Patrick Steinhardt wrote: > > > 57 files changed, 251 insertions(+), 73 deletions(-) > > > > I took a careful read through these patches, and found most of them easy > > to review. I was admittedly a little lost with the "fix various leak" > > patches, and having slightly more context in the commit descriptions > > there would have been helpful. > > Yeah, I was a bit torn here on whether to expand the commit messages or > not. I just didn't think it's all that useful to always say "Variable x > is allocated, but never freed" for trivial cases where allocation and > freeing need to happen in the same function, much less so if we repeat > such commits for every single such variable in the same subsystem. So I > just threw those fixes into a single bag. I think sometimes that level of detail is useful, e.g., for tracking where the leak came from, if it's always existed, etc. But I agree enumerating each leak in an otherwise straightforward commit is not a good use of author or reviewer time. I think more useful would be enumerating the kinds of leaks that you clean up. Thanks, Taylor