<deque>: Properly destroy (fancy) pointers to blocks in the internal map - #2775
Conversation
The map array is fully constructed just after allocation, so it should be fully destroyed before deallocation.
|
|
||
| size_t fancy_counter{}; | ||
|
|
||
| template <class T> |
There was a problem hiding this comment.
That is some pretty extensive work needed for a small test.
I am wondering whether it would make sense to reuse that machinery and test all containers for internal memory leaks?
Co-authored-by: Michael Schellenberger Costa <mschellenbergercosta@gmail.com>
Per @miscco 's comment.
Also place _Mysize() before _Newsize for consistency
| void _Construct_n(size_type _Count, const _Ty& _Val) { // construct from _Count * _Val | ||
| _Tidy_guard<deque> _Guard{this}; | ||
| for (; 0 < _Count; --_Count) { | ||
| for (; _Count > 0; --_Count) { |
There was a problem hiding this comment.
No change requested: We generally try to avoid mixing unrelated cleanups into behavioral changes (as this increases the difficulty of reviewing and understanding source history). This isn't an ironclad rule - we're human and it's easier to fold changes into an existing PR, so a small amount of cleanup can be fine. In this case I think it's fine, but I wanted to mention it.
| friend bool operator==(const counting_ptr& lhs, const counting_ptr& rhs) = default; | ||
|
|
||
| friend auto operator<=>(const counting_ptr& lhs, const counting_ptr& rhs) = default; |
There was a problem hiding this comment.
No change requested: In product code, we conventionally don't name parameters of defaulted functions. On the other hand, in test code we're somewhat less rigorous about conventions. On the third hand, operator=(const counting_ptr&) didn't name its parameter, which is an argument for being locally consistent here. On the fourth and final hand, this isn't worth resetting testing. 😹
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
|
I've resolved a trivial adjacent-add merge conflict in |
|
Thanks for fixing this correctness bug! 🐞 🎉 😸 |
…l map (microsoft#2775) Co-authored-by: Michael Schellenberger Costa <mschellenbergercosta@gmail.com> Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
4 participants
The internal map array is fully constructed, so it should also be fully destroyed before deallocation.
Fixes #2769.