Fix issue #274 UBSan Error constructor call with insufficient space for an object of type 'node_t'#323
Conversation
arximboldi
left a comment
arximboldi
left a comment
There was a problem hiding this comment.
Super nice, thank you! There are some comments inline with some small requests, could you please take care of those?
| typename aligned_storage<sizeof(T), alignof(T)>::type; | ||
|
|
||
| template <typename Base, typename T, bool EmptyBase = false> | ||
| struct has_trailing_storage; |
There was a problem hiding this comment.
Nitpick: the has_ verb makes it looks like this is a meta-predicate (a boolean returning meta-function). For "mix-ins" of this form I prefer the with_ prefix, as in:
struct with_trailing_storage;There was a problem hiding this comment.
Also a comment explaining why and how this is needed would be nice to have.
There was a problem hiding this comment.
Agreed, with_trailing_storage is a much better name. I added some comments, let me know if you are OK with them or if you wanted to explain why this is needed instead of the original aligned_storage solution.
| template <typename Base, typename T, bool EmptyBase = false> | ||
| struct has_trailing_storage; | ||
|
|
||
| template <typename Base, typename T> |
There was a problem hiding this comment.
Nitpick: Base should be rename the Deriv, as it is not the base clase but the class that derives from this (technically, with_trailing_storage is the base class of Deriv as usual in CRTP)
There was a problem hiding this comment.
Haha true, I did it completely backwards. Fixed.
| static constexpr void check_base() | ||
| { | ||
| static_assert(std::is_standard_layout<Base>::value, | ||
| "Please remove 'true' if the base class is non emtpy"); |
There was a problem hiding this comment.
Pretty neat trick to discover when this still holds :)
|
Thank you for the name change and the comments. This is looking great; thank you so much! Merging :) |
|
Hello @arximboldi, just to clarify, did you forget to actually merge or is there some kind of schedule? |
|
Sorry, was waiting for CI and then forgot. It works (the failing test is because you're a third-party contributor it doesn't expose secrets, I should fix the actions to ignore those steps in that case). Merging! |
2 participants
This PR adds the CRTP struct
with_trailing_storage. Any standard layout struct can inherit from it to be able to use the storage immediately after it to store objects of a given type. For efficiency and to accommodate other immer facilities, there is a special case for an empty base class.