search

shadow-maint / shadow

Upstream shadow tree
Other
299 stars 230 forks source link

Fix 2 memory leaks #996

Closed alejandro-colomar closed 4 months ago

alejandro-colomar commented 4 months ago

Hi @ikerexxe ,

This is my implementation with a goto, plus additional refactors to address your concerns about goto, and also simplify some weirdness.

alejandro-colomar commented 4 months ago

Very good improvement. Thank you man!

Thanks! :-)

I added two inline comments, and I'd also like you to change two commits messages: 14aa5a8 and 1ea4f91. The code explanation is too much, I think it's enough with the first two lines.

The scripts that I show in the commit messages and their output are interesting to review the commit. Otherwise, given the length of the function, it might be more difficult to review. Also, having the scripts in a commit message, can serve to reuse them in similar cases, so we can use it as a cheat sheet.

In general, I find refactors to be error-prone, and thus I like putting more information to demonstrate the validity of the change. And if we come back to git-blame(1) some of these refactors in 10 years from now, I think we'll thank having these scripts, to not need to verify the validity of the patches.