pfr_fields_view

[denzor200]

This PR is a part of the issue #231.

[denzor200]

Here is just docs for "pfr_fields_view"..
Implementation in progress..

[denzor200]

Docs for "pfr_view" and for "BOOST_FUSION_ADAPT_PFR" extension will be in another PRs.

[djowel]

This will introduce a dependency. I am not sure... We should think about this more. Hopefully, it is decoupled in a separate include?

[denzor200]

This will introduce a dependency.

You mean dependency of Fusion from Boost.PFR Library, right?

Hopefully, it is decoupled in a separate include?

Yes, all pfr_view's must be in a separate include to preserve backward compatibility, cause of Boost.PFR an C++14 library.

This fact is documented everywhere like this:

[heading Header]

    #include <boost/fusion/view/pfr_fields_view.hpp>
    #include <boost/fusion/include/pfr_fields_view.hpp>

[note This view won't defined by inclusion of 'boost/fusion/view.hpp'. ]

[denzor200]

@djowel, fixed

[daminetreg]

I just wanted to add that I'm watching this from far (not much time sadly at the moment).

And wanted to say this is really awesome and I love it (as last big contributor on BOOST_FUSION_ADAPT_STRUCT). I did a pfr integration in this direction for my talk at CppCon last year, happy @denzor200 you are doing that cleanly ready for prime time.

It makes boost fusion work out of the box for many types and many automations based on Fusion work seamlessly with types that aren't "ADAPTED".

The future is in this PR. 👍

[djowel]

Are we ready for review and merge?

[djowel]

I just wanted to add that I'm watching this from far (not much time sadly at the moment).

And wanted to say this is really awesome and I love it (as last big contributor on BOOST_FUSION_ADAPT_STRUCT). I did a pfr integration in this direction for my talk at CppCon last year, happy @denzor200 you are doing that cleanly ready for prime time.

It makes boost fusion work out of the box for many types and many automations based on Fusion work seamlessly with types that aren't "ADAPTED".

The future is in this PR. 👍

How about doing a review @daminetreg ?

[denzor200]

Are we ready for review and merge?

PR is ready for review and merge. I have no planned changes

[daminetreg]

I would like to apologize for the delay. I'll happily make a first review.

[denzor200]

Hello everyone, how are you? Do you have an interest/opportunity to continue moving within this PR?

[daminetreg]

hello yes I’ll review this weekend very sorry for the big delay

[daminetreg]

I started reviewing but will need a bit more time, before being able to push the first comments, I started using the pfr_view in test programs. Looks cool. :-) I'll come back to you here by the end of the week.

[denzor200]

Still I have the feeling this could get one step further, wouldn't it be possible to select via SFINAE non adapted types and check if its autoadapted via Boost.Pfr instead of requiring users to call BOOST_FUSION_ADAPT_PFR ?

That would be nice, but I haven't found a way to do this with clean changes yet. I really would not like to make changes in the core of Boost Fusion. But, if your request is a serious request, I can continue the search how to do it in the near future.

Also, don't forget that your 'step further' is more likely to regress than the implementation I've shown here. Even if it will be a super clean changes.

[denzor200]

Providing for example a default template for all the base metafunction: at_impl, begin_impl... an implementation that would use PFR if it's non adapted manually, without requiring the use of a separate macro call to register the type.

If I understand you correctly, instead of a macro, you propose to implement specializations of the at_impl, begin_impl and so other methods for the non_fusion_tag tag.

This will most likely work and the custom struct will be serialized by Fusion without BOOST_FUSION_ADAPT_PFR. But there is a nuance - such a structure will not be a legal Fusion Sequence(*). At the very least, this can cause problems using this structure in generic code.

I propose to do the automatic serialization you suggested, but leave my version as a backup for a difficult situation. In total, there will be three ways to serialize a structure:

• Automatic
• fusion::pfr
• fusion::pfr_fields

We'll remove the BOOST_FUSION_ADAPT_PFR macro as unnecessary.

@daminetreg What do you think about this?

(* for this you need to know that this structure can be serialized via PFR, otherwise I do not have the right to specialize is_sequence for all non_fusion_tag. This issue was discussed here boostorg/pfr#56, but the movement on it No, and I'm not sure that it will be possible to implement it in an acceptable form.)

[denzor200]

Providing for example a default template for all the base metafunction: at_impl, begin_impl... an implementation that would use PFR if it's non adapted manually, without requiring the use of a separate macro call to register the type.

Unfortunately, it didn't work out. As it turns out, many Fusion methods require the argument to match the Sequence concept, fusion::begin is one of them.
https://godbolt.org/z/jzWrxcorc

I do not deny that this could be solved through the fusion::begin function overload, but it already looks like a crutch and I doubt that it will pass the review.
I suggest to pass BOOST_FUSION_ADAPT_PFR macro and don't remove it yet. The remaining comments on the review will be fixed in the near future.

[denzor200]

This PR ready for review. Another PRs(pfr view, pfr extension) will be actualized in the near future.

[Kojoley]

The problem of this PR (omitting additional dependencies it brings) is addition of pfr_fields function, which should not exist, because every place where pfr_fields is called it indicates a lack of implementation for existing Fusion functions.

[denzor200]

The problem of this PR (omitting additional dependencies it brings) is addition of pfr_fields function, which should not exist, because every place where pfr_fields is called it indicates a lack of implementation for existing Fusion functions.

I understand correctly that you also as @daminetreg propose to upgrade Fusion to automatically use PFR on non adapted types?

Like this:

AnUserDefinedStructure attr;
*fusion::begin(attr) = 0;

Instead of this:

AnUserDefinedStructure attr;
*fusion::begin(fusion::pfr_fields(attr)) = 0;

Correct me if I misunderstood you.

[Kojoley]

The problem of this PR (omitting additional dependencies it brings) is addition of pfr_fields function, which should not exist, because every place where pfr_fields is called it indicates a lack of implementation for existing Fusion functions.

I understand correctly that you also as @daminetreg propose to upgrade Fusion to automatically use PFR on non adapted types?

Like this:

AnUserDefinedStructure attr;
*fusion::begin(attr) = 0;

Instead of this:

AnUserDefinedStructure attr;
*fusion::begin(fusion::pfr_fields(attr)) = 0;

Correct me if I misunderstood you.

Pretty much yes, though I still think the support should be opt-in (by including some enabling header first).

[denzor200]

Pretty much yes, though I still think the support should be opt-in (by including some enabling header first).

Why not, but then an attribute will be passed to Spirit that's not a Sequence, but at the same time has overloaded methods (fusion::begin, fusion::at, ...). Is Spirit able to work with such an attribute?

[denzor200]

template<typename Attrubute>
void verify_sequence(const Attrubute&) {
   static_assert(fusion::traits::is_sequence< Attrubute >::value, "Not a legal Fusion Sequence");
}

AnUserDefinedStructure attr;
*fusion::begin(fusion::pfr_fields(attr)) = 0;
verify_sequence( fusion::pfr_fields(attr) );  //< GOOD

AnUserDefinedStructure attr;
*fusion::begin(attr) = 0;
// verify_sequence( attr );  //< anyway will failure

We use fusion::pfr_fields to tell that our attribute is a legal Sequence, we have no way to determine it automatic, PFR can not this. This issue was discussed here boostorg/pfr#56, but the movement on it No, and I'm not sure that it will be possible to implement it in an acceptable form.

[Kojoley]

Looking at https://github.com/boostorg/pfr/blob/develop/include/boost/pfr/detail/fields_count.hpp I feel like it is very much possible (and it also seems possible to make PFR work with empty bases classes).

[denzor200]

I feel like it is very much possible

You suggest to resolve boostorg/pfr#56 on the PFR side?

and it also seems possible to make PFR work with empty bases classes

I doubt that it will be possible to do this in c++14 https://godbolt.org/z/nvvzrTr9G (off-top)

[denzor200]

I created pfr ticket: boostorg/pfr#100 and i works with PR for this ticket.
This pfr functional will let us to implement auto detection serializable types(if we really need it).
Also, this will let us to remake this PR more shorter, because there will be more ability in the pfr side.

[denzor200]

I created pfr ticket: boostorg/pfr#100 and i works with PR for this ticket. This pfr functional will let us to implement auto detection serializable types(if we really need it). Also, this will let us to remake this PR more shorter, because there will be more ability in the pfr side.

I doned docs and posted it here: boostorg/pfr#107
Everyone who wish can participate in review. Other things - implementation and tests are in progress and will be available soon.

[xR3b0rn]

Is there any progress on this?

[djowel]

Is there any progress on this?

Indeed, I'd love to get this merged soon. Are there any showstoppers that need to be addressed, that are preventing merge?

[denzor200]

Is there any progress on this?

Of course, I'm working on it now on the PFR side, then I will return to the Fusion and this PR will be actualized(or recreated).
I'd like to complete this work, and to fulfill all the previous wishes from reviewers.

Are there any showstoppers that need to be addressed, that are preventing merge?

We have to wait when boostorg/pfr#111 and boostorg/pfr#86 will be accepted and merged into PFR. And till we waiting, we can do something with broken CI, I suggested a fix here: #262

[denzor200]

Hello everyone, I have a good news, both PRs(boostorg/pfr#111 and boostorg/pfr#86) successfully merged into PFR, I'm ready for switch to working on Fusion. This PR will be reacreated in a very very nearest future.

[denzor200]

@djowel can we merge #262 to apply fix for CI?

[djowel]

@djowel can we merge #262 to apply fix for CI?

Done.

[denzor200]

closed because not actual.
Actual PR here: #266