Do not use std::aligned_storage#47
Conversation
[why] We should verify that SmallVector actually places its elements according to their alignment requirements. As it turns out, it does. :)
[why] std::aligned_storage is deprecated since C++23, see: https://stackoverflow.com/questions/71828288/why-is-stdaligned-storage-to-be-deprecated-in-c23-and-what-to-use-instead [how] Use alignas() for the internal buffer and specify the alignment explicitly when calling new for an allocated buffer. As it turns out, an aligned new[] has to be deallocated with an aligned delete[].
alt-graph
requested review from
Seim2k17,
Copilot,
ohensler,
soerengrunewald and
zykure
There was a problem hiding this comment.
Pull request overview
This PR updates gul17::SmallVector to avoid using std::aligned_storage (deprecated in C++23) by switching to an alignas(...)-aligned internal byte buffer and using aligned new[] / delete[] for heap storage. It also adds a unit test to validate alignment after growth and shrink_to_fit().
Changes:
- Replace
std::aligned_storagewith analignas(ValueType)internalstd::bytebuffer and aligned heap allocation/deallocation. - Add an alignment regression test that exercises
push_back()growth andshrink_to_fit()transitions. - Update changelog/copyright headers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
include/gul17/SmallVector.h |
Reworks internal/heap storage to avoid std::aligned_storage and uses aligned new[]/delete[]. |
tests/test_SmallVector.cc |
Adds a new template test to verify element alignment and contiguity across growth/shrink. |
data/doxygen.h |
Updates the “Unreleased” changelog section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void deallocate_storage() noexcept | ||
| { | ||
| if (!is_storage_allocated()) | ||
| return; | ||
|
|
||
| ::operator delete[](data_ptr_, alignment); | ||
| } |
There was a problem hiding this comment.
gcc with -fanalyzer points this out as well.
There was a problem hiding this comment.
The Copilot "fix" is bull****, but I see how leaving the pointer dangling at the end of the function may arouse suspicion. I'll add a commit in which the pointer is set to null if a deallocation took place and I'll state explicitly that the class invariants are temporarily broken when this happens.
The only other way out I see here is to remove the helper function again and inline the instructions, as it was before.
There was a problem hiding this comment.
The only other way out I see here is to remove the helper function again and inline the instructions, as it was before.
With the latest commit I have taken that route. It is probably better than having a member function that breaks the class invariants, even if it is private.
[why] The function could leave data_ptr_ dangling after the call. This was clearly documented and logically OK within the class, but could raise suspicion. [how] Explicitly document that the function may leave the class in a state in which the class invariants are violated. If deallocation takes place, assign nullptr to data_ptr_ so that there is a better chance to find misuses.
[why]
MSVC mistakes the statement
allocation = new (alignment) std::byte[new_capacity * sizeof(ValueType)];
for a placement new even if alignment is of the type std::align_val_t,
and therefore generates a compilation error.
[how]
Use the low-level form of new directly like this:
auto* ptr = static_cast<std::byte*>(
::operator new[](num_elements * sizeof(ValueType), alignment));
Because this is ugly, extract both allocation and deallocation into
static member functions.
[why] Having a private member function that breaks the class invariants may be confusing, and it does not make things any clearer than having the two lines directly in the code.
alt-graph
force-pushed
the
cleanup/do_not_use_std_aligned_storage
branch
from
f3533f0 to
7c0ab98
Compare
[why]
std::aligned_storageis deprecated since C++23, see: https://stackoverflow.com/questions/71828288/why-is-stdaligned-storage-to-be-deprecated-in-c23-and-what-to-use-instead[how]
Use
alignas()for the internal buffer and specify the alignment explicitly when callingnewfor an allocated buffer. As it turns out, an alignednew[]has to be deallocated with an aligneddelete[], so we have to change that handling, too. We also had no unit test for the alignment requirements, so we add one.