Skip to content
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

View manipulator and is_reflectable trait #107

Closed
wants to merge 15 commits into from

Conversation

denzor200
Copy link
Contributor

@denzor200 denzor200 marked this pull request as draft September 9, 2022 19:45
@Kojoley
Copy link
Contributor

Kojoley commented Sep 9, 2022

I don't want to devalue your work, after all thanks for at least trying to work on this. Probably you misunderstood me, it seems that is_reflectable is just a placeholder? It actually does nothing by default? Maybe I forgot to show you that there is already in the library an almost working trait

template <class T, std::size_t N>
struct is_aggregate_initializable_n {
template <std::size_t ...I>
static constexpr bool is_not_constructible_n(std::index_sequence<I...>) noexcept {
return (!std::is_constructible<T, decltype(ubiq_lref_constructor{I})...>::value && !std::is_constructible<T, decltype(ubiq_rref_constructor{I})...>::value)
|| is_single_field_and_aggregate_initializable<N, T>::value
;
}
static constexpr bool value =
std::is_empty<T>::value
|| std::is_array<T>::value
|| std::is_fundamental<T>::value
|| is_not_constructible_n(detail::make_index_sequence<N>{})
;
};
your best bet is to fix some of it's blind spots and it solves the exact problem. Or you have tried to fix it and came to conclusion that it's not possible?

Also, a general rule: the less code is for a review the faster your PR is getting reviewed and more chances for it to be merged in a timely manner.

Copy link
Member

@apolukhin apolukhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nitpicks


There are three ways to start using Boost.PFR from other library for type `T` in your code. Each method has its own drawbacks and suits own cases.

[table:ops_comp Different approaches for interaction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the table id to something unique, for example to table:libs_integration_comp


This method is good if you're writing generic algorithms using a library with Boost.PFR support and need to use reflection from Boost.PFR only if the type can't be recognized by the library:

```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some day this code snippet should be moved into cpp file and compiled like pfr_sample_adapting

[[
[headerref boost/pfr/view.hpp boost/pfr/view.hpp: view]
][
Use when you need to access values by library's provided for them reflection or via field-by-field reflection.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Use when you need to access values by library's provided for them reflection or via field-by-field reflection.
Use when you need to access values by other library provided reflection or via field-by-field reflection from Boost.PFR.

?

@denzor200
Copy link
Contributor Author

Probably you misunderstood me, it seems that is_reflectable is just a placeholder? It actually does nothing by default?

I do understand you about your issue automatic is_reflectable, this PR won't make to resolve issue impossible, issue will be resolved in the future version of PFR.

@denzor200
Copy link
Contributor Author

Not actual. Actual PR here: #111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants