Fix etl::typed_storage by supporting omitted destructors

[rolandreichweinbmw]

In a recent change to alignment.h, the etl::typed_storage was changed in a way that in case of an already constructed object, the object is created via assignment.

However, this contradicts the original use case that led to etl::typed_storage in the first place:
#1023

The goal is to omit destructors (and at the same time support classes with deleted assignment operators), so they can be optimized out at link time.

This change reverts commit ac7b268 to restore the original functionality and changes the test to reflect the original use case.

[coderabbitai]

Warning

Rate limit exceeded

@rolandreichweinbmw has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 20 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 3a5f60f and 980d217.

📒 Files selected for processing (1)

• include/etl/alignment.h (7 hunks)

Walkthrough

The PR standardises typed_storage::create overloads (C++11 and pre-C++11) in include/etl/alignment.h to always assert !has_value(), perform placement-new into the designated storage (internal or external), set valid, and return a reference to the newly constructed object. typed_storage_ext constructors now delegate to create(...) for non-CPP11 paths and construct in the external buffer; destruction uses the external buffer when valid. Error handling uses ETL_ASSERT(..., ETL_ERROR(etl::typed_storage_error)). Tests add struct A_t with a destructor, deleted copy-assignment and a const-correct equality operator. No public API declarations changed.

✨ Finishing Touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

include/etl/alignment.h (1)

710-715: typed_storage_ext: same valid-before-construct issue

Reorder to set valid after successful placement-new to keep state consistent if construction throws.

   reference create(TArgs&&... args) ETL_NOEXCEPT_EXPR(ETL_NOT_USING_EXCEPTIONS)
   {
     ETL_ASSERT(!has_value(), ETL_ERROR(etl::typed_storage_error));
-    valid = true;
-    return *::new (pbuffer) value_type(etl::forward<TArgs>(args)...);
+    value_type* p = ::new (pbuffer) value_type(etl::forward<TArgs>(args)...);
+    valid = true;
+    return *p;
   }

   reference create(const T1& t1)
   {
     ETL_ASSERT(!has_value(), ETL_ERROR(etl::typed_storage_error));
-    valid = true;
-    return *::new (pbuffer) value_type(t1);
+    value_type* p = ::new (pbuffer) value_type(t1);
+    valid = true;
+    return *p;
   }

   reference create(const T1& t1, const T2& t2)
   {
     ETL_ASSERT(!has_value(), ETL_ERROR(etl::typed_storage_error));
-    valid = true;
-    return *::new (pbuffer) value_type(t1, t2);
+    value_type* p = ::new (pbuffer) value_type(t1, t2);
+    valid = true;
+    return *p;
   }

   reference create(const T1& t1, const T2& t2, const T3& t3)
   {
     ETL_ASSERT(!has_value(), ETL_ERROR(etl::typed_storage_error));
-    valid = true;
-    return *::new (pbuffer) value_type(t1, t2, t3);
+    value_type* p = ::new (pbuffer) value_type(t1, t2, t3);
+    valid = true;
+    return *p;
   }

   reference create(const T1& t1, const T2& t2, const T3& t3, const T4& t4)
   {
     ETL_ASSERT(!has_value(), ETL_ERROR(etl::typed_storage_error));
-    valid = true;
-    return *::new (pbuffer) value_type(t1, t2, t3, t4);
+    value_type* p = ::new (pbuffer) value_type(t1, t2, t3, t4);
+    valid = true;
+    return *p;
   }

Also applies to: 724-727, 736-739, 748-751, 760-763

🧹 Nitpick comments (2)

test/test_alignment.cpp (2)

70-74: Delete the canonical copy-assignment signature

To robustly block assignment, prefer deleting the standard signature taking const A_t&.

-  A_t& operator=(A_t& other) = delete;
+  A_t& operator=(const A_t&) = delete;

---

75-78: Make operator== const-correct

Allows comparisons of const instances and matches conventional semantics.

-  bool operator==(A_t& other)
+  bool operator==(const A_t& other) const
   {
     return other.x == x && other.y == y;
   }

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7324388 and f3c8151.

📒 Files selected for processing (2)

• include/etl/alignment.h (5 hunks)
• test/test_alignment.cpp (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

include/etl/*.h

⚙️ CodeRabbit configuration file

include/etl/*.h: Review the C++ code in these directories. The code must not use
STL containers but must instead rely on Embedded Template Library
(ETL) for data structures and algorithms.

When answering questions, provide accurate and concise information based on the ETL documentation and codebase. If you don't know the answer, just say "I don't know". Do not make up answers.
When providing code examples, ensure they are relevant to the ETL and follow its conventions.
Always be polite and professional in your responses.

Header files should start with the ETL's standard MIT comment block.

Header files should have include guards in the format '#ifndef ETL_>INCLUDED #define ETL_INCLUDED #endif'

Headers should be organized in the following order

• The first header include should be '#include "platform.h"'
• Headers from the ETL
• Headers from the path 'private/**'
• C++ standard library headers
• C standard library headers

Pay extra attention to the following

• Buffer overflows
• Memory leaks
• Undefined behavior

Check that the following conventions are used

• Variable names should be in snake_case
• Macro constants should be ALL_CAPS_WITH_UNDERSCORES
• Macro constants should be prefixed with 'ETL_'
• Function, class, and type names should be snake_case
• enum members should be Caps_Snake_Case

Please keep your outputs short and to the point, unless otherwise asked

• Keep your responses short, describe small issues in a few sentences
• Don't output tips, analysis chains or anything else with a collapsible view
• Don't output comment summaries or fix prompts for AIs
• If the highlighted issue is complex, don't suggest a solution
• Don't use emojis

Files:

• include/etl/alignment.h

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)

• GitHub Check: GCC C++11 Linux - No STL (ubuntu-22.04)
• GitHub Check: Clang C++23 Linux - STL - Force C++03 (ubuntu-22.04)
• GitHub Check: Clang C++23 Linux - No STL (ubuntu-22.04)
• GitHub Check: Clang C++17 Linux - No STL (ubuntu-22.04)
• GitHub Check: Clang C++20 Linux - STL - Force C++03 (ubuntu-22.04)
• GitHub Check: GCC C++23 Linux - STL - Force C++03 (ubuntu-22.04)
• GitHub Check: GCC C++23 Linux - No STL - Force C++03 (ubuntu-22.04)
• GitHub Check: GCC C++23 Linux - STL (ubuntu-22.04)
• GitHub Check: Clang C++20 Linux - No STL (ubuntu-22.04)
• GitHub Check: GCC C++20 Linux - No STL - Force C++03 (ubuntu-22.04)
• GitHub Check: GCC C++20 Linux - No STL (ubuntu-22.04)
• GitHub Check: GCC C++23 Linux - No STL (ubuntu-22.04)
• GitHub Check: GCC C++17 Linux - STL (ubuntu-22.04)
• GitHub Check: GCC C++20 Linux - STL (ubuntu-22.04)
• GitHub Check: GCC C++20 Linux - STL - Force C++03 (ubuntu-22.04)
• GitHub Check: Clang C++14 Linux - No STL (ubuntu-22.04)
• GitHub Check: Clang C++14 Linux - STL (ubuntu-22.04)
• GitHub Check: Clang C++11 Linux - No STL (ubuntu-22.04)
• GitHub Check: Clang C++11 Linux - STL (ubuntu-22.04)
• GitHub Check: GCC C++17 Linux - No STL (ubuntu-22.04)

🔇 Additional comments (2)

test/test_alignment.cpp (2)

51-59: LGTM: clear demonstrator type for typed_storage tests

Definition and ctor are straightforward and appropriate for the test intent.

---

60-69: Destructor-with-side-effects suits the test case

Explicit dtor helps verify that etl::typed_storage does not implicitly destroy. No issues spotted.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

include/etl/alignment.h (1)

646-652: Non-C++11 single-arg ctor doesn't construct the value

typed_storage_ext(void* pbuffer_, const T1& t1) omits create(t1); the 2/3/4-arg ctors do call create(...). This leaves valid=false even though a value is expected to be constructed.

Apply:

     typed_storage_ext(void* pbuffer_, const T1& t1)
       : pbuffer(reinterpret_cast<T*>(pbuffer_))
       , valid(false)
     {
       ETL_ASSERT(etl::is_aligned(pbuffer_, etl::alignment_of<T>::value), ETL_ERROR(etl::alignment_error));
+      create(t1);
     }

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f3c8151 and 0562f6a.

📒 Files selected for processing (2)

• include/etl/alignment.h (10 hunks)
• test/test_alignment.cpp (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

include/etl/*.h

⚙️ CodeRabbit configuration file

include/etl/*.h: Review the C++ code in these directories. The code must not use
STL containers but must instead rely on Embedded Template Library
(ETL) for data structures and algorithms.

When answering questions, provide accurate and concise information based on the ETL documentation and codebase. If you don't know the answer, just say "I don't know". Do not make up answers.
When providing code examples, ensure they are relevant to the ETL and follow its conventions.
Always be polite and professional in your responses.

Header files should start with the ETL's standard MIT comment block.

Header files should have include guards in the format '#ifndef ETL_>INCLUDED #define ETL_INCLUDED #endif'

Headers should be organized in the following order

• The first header include should be '#include "platform.h"'
• Headers from the ETL
• Headers from the path 'private/**'
• C++ standard library headers
• C standard library headers

Pay extra attention to the following

• Buffer overflows
• Memory leaks
• Undefined behavior

Check that the following conventions are used

• Variable names should be in snake_case
• Macro constants should be ALL_CAPS_WITH_UNDERSCORES
• Macro constants should be prefixed with 'ETL_'
• Function, class, and type names should be snake_case
• enum members should be Caps_Snake_Case

Please keep your outputs short and to the point, unless otherwise asked

• Keep your responses short, describe small issues in a few sentences
• Don't output tips, analysis chains or anything else with a collapsible view
• Don't output comment summaries or fix prompts for AIs
• If the highlighted issue is complex, don't suggest a solution
• Don't use emojis

Files:

• include/etl/alignment.h

🧠 Learnings (1)

📓 Common learnings

Learnt from: filipe-cuim
PR: ETLCPP/etl#1174
File: include/etl/type_traits.h:1974-1985
Timestamp: 2025-09-01T14:17:35.361Z
Learning: The current implementation in this PR correctly uses only individual trait flags (like ETL_USING_BUILTIN_IS_ASSIGNABLE) in preprocessor guards, not the global ETL_USE_TYPE_TRAITS_BUILTINS flag, because the global flag automatically defines all individual flags when enabled, making dual checks redundant.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)

• GitHub Check: Clang C++20 OSX - STL - Force C++03 (macos-13)
• GitHub Check: GCC C++23 Linux - STL - Force C++03 (ubuntu-22.04)
• GitHub Check: GCC C++23 Linux - No STL (ubuntu-22.04)
• GitHub Check: GCC C++23 Linux - No STL - Force C++03 (ubuntu-22.04)
• GitHub Check: GCC C++23 Linux - STL (ubuntu-22.04)
• GitHub Check: Clang C++23 Linux - No STL (ubuntu-22.04)
• GitHub Check: Windows - STL - Force C++03
• GitHub Check: Clang C++23 Linux - No STL - Force C++03 (ubuntu-22.04)
• GitHub Check: Clang C++23 Linux - STL - Force C++03 (ubuntu-22.04)
• GitHub Check: Clang C++11 Linux - No STL (ubuntu-22.04)
• GitHub Check: Clang C++11 Linux - STL (ubuntu-22.04)
• GitHub Check: Clang C++14 Linux - STL (ubuntu-22.04)
• GitHub Check: Clang C++14 Linux - No STL (ubuntu-22.04)
• GitHub Check: GCC C++11 Linux - STL (ubuntu-22.04)
• GitHub Check: GCC C++11 Linux - No STL (ubuntu-22.04)
• GitHub Check: Clang C++17 Linux - STL (ubuntu-22.04)
• GitHub Check: GCC C++14 Linux - STL (ubuntu-22.04)
• GitHub Check: GCC C++17 Linux - STL (ubuntu-22.04)
• GitHub Check: GCC C++14 Linux - No STL (ubuntu-22.04)
• GitHub Check: GCC C++17 Linux - No STL (ubuntu-22.04)

🔇 Additional comments (3)

test/test_alignment.cpp (1)

51-75: Good demonstrator type for the deleted-assignment/omitted-destructor use case

The deleted copy-assignment and const-correct equality operator exercise the intended semantics for typed_storage. The destructor side-effect helps validate destroy() paths in tests.

include/etl/alignment.h (2)

453-457: Exception-safety order is now correct

valid is set only after successful placement-new. This prevents has_value() becoming true if construction throws. LGTM.

---

466-470: Consistent construction path across overloads

All these create() overloads assert !has_value(), perform placement-new, then set valid and return a reference. Matches the intended revert and supports types with deleted assignment. LGTM.

Also applies to: 490-496, 502-509, 718-721, 731-734, 744-747, 757-760, 770-773

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

include/etl/alignment.h (1)

479-483: create(T1, T2) fix retained and semantics restored

The prior return “:” typo is gone and the function now mirrors the safe pattern.

🧹 Nitpick comments (1)

include/etl/alignment.h (1)

712-714: Doc string says “internal byte array” for external storage

Minor wording nit: this is an external buffer.

-    /// \returns the instance of T which has been constructed in the internal byte array.
+    /// \returns the instance of T which has been constructed in the external buffer.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0562f6a and 3a5f60f.

📒 Files selected for processing (2)

• include/etl/alignment.h (11 hunks)
• test/test_alignment.cpp (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• test/test_alignment.cpp

🧰 Additional context used

📓 Path-based instructions (1)

include/etl/*.h

⚙️ CodeRabbit configuration file

include/etl/*.h: Review the C++ code in these directories. The code must not use
STL containers but must instead rely on Embedded Template Library
(ETL) for data structures and algorithms.

When answering questions, provide accurate and concise information based on the ETL documentation and codebase. If you don't know the answer, just say "I don't know". Do not make up answers.
When providing code examples, ensure they are relevant to the ETL and follow its conventions.
Always be polite and professional in your responses.

Header files should start with the ETL's standard MIT comment block.

Header files should have include guards in the format '#ifndef ETL_>INCLUDED #define ETL_INCLUDED #endif'

Headers should be organized in the following order

• The first header include should be '#include "platform.h"'
• Headers from the ETL
• Headers from the path 'private/**'
• C++ standard library headers
• C standard library headers

Pay extra attention to the following

• Buffer overflows
• Memory leaks
• Undefined behavior

Check that the following conventions are used

• Variable names should be in snake_case
• Macro constants should be ALL_CAPS_WITH_UNDERSCORES
• Macro constants should be prefixed with 'ETL_'
• Function, class, and type names should be snake_case
• enum members should be Caps_Snake_Case

Please keep your outputs short and to the point, unless otherwise asked

• Keep your responses short, describe small issues in a few sentences
• Don't output tips, analysis chains or anything else with a collapsible view
• Don't output comment summaries or fix prompts for AIs
• If the highlighted issue is complex, don't suggest a solution
• Don't use emojis

Files:

• include/etl/alignment.h

🧬 Code graph analysis (1)

include/etl/alignment.h (2)

include/etl/private/variant_legacy.h (1)

• etl (57-690)

include/etl/variant_pool.h (1)

• etl (62-369)

🔇 Additional comments (10)

include/etl/alignment.h (10)

453-456: Exception-safe create() with placement-new: LGTM

Restores correct semantics (no assignment to an already-constructed object) and sets valid only after successful construction. Supports deleted assignment and omitted destructors again.

---

466-470: Pre-C++11 create(T1): LGTM

Consistent with the C++11 path: placement-new then set valid, return reference.

---

492-496: Pre-C++11 create(T1, T2, T3): LGTM

Correct order: construct, mark valid, return.

---

505-509: Pre-C++11 create(T1, T2, T3, T4): LGTM

Matches the intended behaviour and exception-safety.

---

652-653: Constructor delegates to create(): LGTM

Removes duplication and keeps the single construction path.

---

718-722: typed_storage_ext::create(...): LGTM

Placement-new into external buffer, valid set after construction, returns reference. Matches internal variant.

---

731-735: Pre-C++11 ext create(T1): LGTM

Consistent and exception-safe.

---

744-748: Pre-C++11 ext create(T1, T2): LGTM

Pattern matches other overloads.

---

757-761: Pre-C++11 ext create(T1, T2, T3): LGTM

---

770-774: Pre-C++11 ext create(T1, T2, T3, T4): LGTM

[rolandreichweinbmw]

Thanks to the coderabbitai review, I also fixed an unrelated bug (missing create()) and typos in alignment.h.

[thomas-herrmann-1]

Sorry, I just have found this pull request, maybe a bit late. It addresses a problem also connected to the discussion https://github.com/ETLCPP/etl/discussions/1170 that I started some weeks ago. The inner structure of typed_storage is not giving any hint on the contained type which is not really easy to use for debugging and further analysis. Therefore I suggested to use the same representation as used for optional. And with the proposed patch I've also fixed the problem of the lately introduced assignment operator. While it wasn't clear to me who actually brings in this patch I haven't converted this into a pull request yet. Maybe the patch
0001-Change-to-typed_storage-without-requiring-operator.patch.txt could be used for reaching both goals?

[rolandreichweinbmw]

Hi @thomas-herrmann-1, your patch was already completely applied to master and released in ETL 20.43.1 :-)

However, John added further changes to alignment.h (see the has_value() branches in the create() members). This unfortunately breaks the original use case for etl::type_storage where we want to optimize out destructors.

This PR #1182 fixes this.

Independently from your already included patch.

[thomas-herrmann-1]

@rolandreichweinbmw thanks for the hint! I haven't seen the PR for my patch and am now a bit confused. When I pointed out to the problem with the newly introduced assignment operator John was responding with:

I overlooked that that I had added new conditions for the type stored.

Therefore I didn't expect any later change like that.