On Sat, Nov 16, 2024 at 04:37:13PM +0100, Rubén Justo wrote: > On Sat, Nov 16, 2024 at 12:24:02PM +0100, Patrick Steinhardt wrote: > > Rubén's review went through all of the patches and his findings have > > been addressed. > > Yes, this iteration looks good to me. > > Two thoughts about the merge: > > First, I'm concerned that we may not have sufficiently documented how > contributors should proceed to prevent new leaks when submitting > patches, and perhaps avoid some unnecessary noise on the list. I > reviewed Documentation/SubmittingPatches and didn't see any mention > about it. Perhaps it would be helpful to add a note about > SANITIZE=leak. I'm unsure if we want to be explicit about this, > though. Nothing really changes with this series -- we already required code to be leak free beforehand, just not in all of our tests. But in any case, providing pointers for how to check for leaks somewhere could be helpful indeed. I think that can happen outside of this series though, also because I'm not quite sure where to slot this in. > Second, in the (hopefully) exceptional cases where new series discover > old leaks that cannot be fixed within the series itself, for whatever > reason, I don't think there is documentation on the use of the > SANITIZE_LEAK prerequisite. Its use is not desirable and documenting > it could be counterproductive, so perhaps it's better to leave its use > suggested on the list when necessary. That's also my take: we should use SANITIZE_LEAK only in exceptional cases. I very much hope that we can avoid introducing new cases of it altogether with the help of long-time contributors by helping newer contributors that happen to uncover such a leak. I guess time will tell, and if we see that this needs to be added fairly regularly we can iterate and add documentation. Patrick
