-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Member pointers functionality #77
base: develop
Are you sure you want to change the base?
Conversation
This may help to resolve this issue: |
Good idea! I''ve simplified it a little bit https://godbolt.org/z/e8rKWrrEs In my implementation problems are the same:
|
|
|
I tried to construct constexpr member pointer getter: GCC and msvc are promising: Caveats:
|
@schaumb interesting. Unfortunatelly, |
I created a gcc ticket, because it is a compiler bug (probably?). msvc constexpr member pointers can be used as template parameter. But:
Not equality problem occures only if the original member pointer is used. But |
@schaumb Let's see where the answer will be faster :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Here's a few nitpicks
/// s.*memptr = 0; | ||
/// \endcode | ||
template<std::size_t I, class T> | ||
inline auto get_memptr(boost::pfr::type_identity<T>) noexcept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change it to
template<std::size_t I, class T>
inline auto get_memptr() noexcept
In that case the type_identity is not required and should be removed. The
template<std::size_t I, class T>
inline auto get_memptr(const T&) noexcept
overload is not required either
std::size_t offset = 0; | ||
std::size_t space = sizeof(T); | ||
|
||
return tie_as_offsets_tuple_impl<T>(detail::make_index_sequence<detail::fields_count<T>()>{}, offset, space); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a namespace to tie_as_offsets_tuple_impl
to avoid ADL
"====================> Boost.PFR: For safety reasons it is forbidden to reflect unions. See `Reflection of unions` section in the docs for more info." | ||
); | ||
|
||
return tie_as_memptrs_tuple_impl<T>(detail::make_index_sequence<detail::fields_count<T>()>{}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a namespace to tie_as_memptrs_tuple_impl
to avoid ADL
"====================> Boost.PFR: Internal error while casting offset to member pointer: overflow was detected" | ||
); | ||
|
||
union { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an UB. Just use std::memcpy. The efficiency would remain the same - compilers know how to optimize std::memcpy
{ | ||
//BOOST_ASSERT(boost::alignment::detail::is_alignment(alignment)); TODO enable | ||
if (size <= space) { | ||
std::size_t p = ~(alignment - 1) & (offset + alignment - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
provide a more clear names for local variables instead of p
and n
test/run/memptr.cpp
Outdated
short id; | ||
short opt; | ||
int value; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a non-trivial type to an aggregate, std:;string
for example
test/run/memptr.cpp
Outdated
@@ -0,0 +1,42 @@ | |||
// Copyright (c) 2021 Denis Mikhailov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this file to the test/core_memptr
dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All new test filed should be moved to test/core_memptr
Conflicts: include/boost/pfr.hpp test/core/run/memptr.cpp
}; | ||
|
||
int main() { | ||
(void)boost::pfr::get_memptr<3>(boost::pfr::type_identity<Foo>{}); // Must be a compile time error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tabs
#include <boost/pfr/core_memptr.hpp> | ||
|
||
#pragma pack(1) | ||
struct Foo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide the same test but with a structure like this:
#pragma pack(1)
struct sample
{
int ch;
int id;
int opt;
int value;
};
In order to test the case when settled alignment doesn't affect the size.
BTW default align is not always sizeof(int)
. You should provide some logic for select appropriate type between int
, long
, etc and for do nothing when nothing was selected for this platform.
Hello Anton. I have some idea and i want to suggest it for you. This patch will add pretty useful functionality for extracting member pointer of some structure. For example:
Its just a draft - not a final code. If you are interested - i will continue for it.