Introduce Storage::forget() to fix memory leak in Matrix::generic_resize()

[nickmertin]

Fixes #1378

Adds a trait method Storage::forget() to clean up the storage while forgetting (i.e., not dropping) the contents. Makes use of this method instead of mem::forget() in the implementations of Reallocator for DefaultAllocator.

[Andlon]

Thanks, this is great! I don't have the head space or time to wrap my head around the nuances of the Reallocator API here at the moment, but I wanted to ask if the documentation needs to be updated to reflect any possible changes in assumptions made. I don't fully understand them yet myself, I'd need a closer look later to try to verify.

[nickmertin]

Thanks! I looked over the documentation and I don't think any changes are necessary, as it maintains the assumption that the elements of the matrix are not dropped. My only question would be whether it would make more sense to move the new method to RawStorage instead of Storage.

[Andlon]

I haven't forgotten this PR, but I haven't yet been able to devote time to it. It came up on Discord though, and @sebcrozet suggested that the forget function needs to be unsafe, and maybe it should be called forget_elements or something along those lines.

I'll try to get back to this soon, but I suspect I'll be quite busy this week.

[nickmertin]

I like the name forget_elements, I think it gets the behaviour across more accurately than what I put right now.

I don't think it should be unsafe, for the same reason that mem::forget isn't unsafe. Safety does not guarantee that destructors are called, so there is nothing unsafe about calling this function. Marking it as unsafe would push obligations of ensuring safety onto everywhere it is called, which could be hard to follow given that it is a trait method and there are several implementations. Perhaps the behaviour and safety attributes just need to be better documented, as is the case for mem::forget.

[tpdickso]

This looks appropriate to me! And I agree, I think a name like forget_elements would be great to help highlight that it's the elements that are being forgotten and not the storage itself (which will be dropped normally absent any other intervention.)

[Andlon]

You're absolutely right about the fact that forget doesn't have to be unsafe @nickmertin! I just have a few minor suggestions, then we can get this merged in time for the next release :)

[Andlon]

As discussed, perhaps change the name to forget_elements.

I think perhaps "cleans up" is a little vague. Perhaps "Drops the storage without ..." ?

[Andlon]

Perhaps add a "SAFETY" comment here and explain that .set_len(0) always satisfies the invariants of .set_len, but the contents are deliberately leaked

(same for other forget impl)

the "SAFETY comment" practice is a relatively recent thing, and much of nalgebra's codebase, to my knowledge, predates this convention. But I think we should try to stick with it going forward, whenever it makes sense!

[nickmertin]

Sounds good, I will make the necessary changes this weekend.