boostorg / spirit

Boost.org spirit module
http://boost.org/libs/spirit
392 stars 161 forks source link

Allow prvalues as attributes #700

Open denzor200 opened 3 years ago

denzor200 commented 3 years ago

I am using the spirit library in conjunction with the fusion library. I need to parse a structure from a string, given that the elements in the string are in reverse order. I use the concept of "view" from the fusion library for this, and I expect the code to look something like this: https://godbolt.org/z/h3KneWzGh

As you can see, the build of the example stops with an error. The fixed variant contains the creation of a temporary view object, and its build succeeded: https://godbolt.org/z/PxdKrsrPh

Essense of my question: can we provide on the library side an ability to use first variant (variant without creating a temporary object)? Is there an objective reason why its won't work?

Kojoley commented 3 years ago

I feel you frustration, we even had issues in Qi with Fusion functions returning proxies for adapt adt. Simply allowing rvalue references as output parameter smells very bad, I am pretty sure we will start receiving bug reports where people accidentally create a non-proxy temporary and pass it as attribute. There is no standardized proxy interface that would allow us to detect such types, but it is not a big problem, we can provide a customization point for this. However, I really do not like making output argument rvalues, sadly C++ misses ability to switch between by value/by reference depending on a deduced template parameter type.

denzor200 commented 3 years ago

sadly C++ misses ability to switch between by value/by reference depending on a deduced template parameter type.

Why not so? https://godbolt.org/z/7b5ce4b4b

Kojoley commented 3 years ago

sadly C++ misses ability to switch between by value/by reference depending on a deduced template parameter type.

Why not so? godbolt.org/z/7b5ce4b4b

That works, but only if a goal is to fix x3::parse free function, otherwise it requires to duplicate every parse function what is a very questionable practice and cruelly inconvenient for a public API. SFINAE on a top level function would definitely make compile error messages more cryptic.

denzor200 commented 2 years ago

@Kojoley, what do you say about this? https://godbolt.org/z/YqfoYTThs Maybe just adding a prvalue function to spirit will solve this issue?

denzor200 commented 2 years ago

@Kojoley, what do you say about this? https://godbolt.org/z/YqfoYTThs Maybe just adding a prvalue function to spirit will solve this issue?

It's a definetly bad idea because here it UB. This example shows how to reproduce "a reference to deleted object" by use suggested approach: https://godbolt.org/z/dEcobKW38

So, this example will be guaranted good only if we just asssign prvalue to constant reference, and only assing without forwarding through prvalue function: https://godbolt.org/z/1jcM5nG44

I also guess that assing with forwarding through static_cast<const Attribute&> will be good but I haven't found confirmation of this yet: https://godbolt.org/z/hja5jT7je

denzor200 commented 2 years ago

@Kojoley, what do you say about this? https://godbolt.org/z/YqfoYTThs Maybe just adding a prvalue function to spirit will solve this issue?

Variant 2: To implement BOOST_SPIRIT_X3_PRVALUE macro https://godbolt.org/z/fM8jWxa9e

denzor200 commented 2 years ago

Why not so? https://godbolt.org/z/7b5ce4b4b

SFINAE is redundant https://godbolt.org/z/cb6sEzW87

denzor200 commented 2 years ago

I feel you frustration, we even had issues in Qi with Fusion functions returning proxies for adapt adt. Simply allowing rvalue references as output parameter smells very bad, I am pretty sure we will start receiving bug reports where people accidentally create a non-proxy temporary and pass it as attribute. There is no standardized proxy interface that would allow us to detect such types, but it is not a big problem, we can provide a customization point for this. However, I really do not like making output argument rvalues, sadly C++ misses ability to switch between by value/by reference depending on a deduced template parameter type.

https://godbolt.org/z/5Mq645vWE It allows rvalue references as output parameter only for fusion::view. For attempting to pass a non-view as rvalue reference in output parameter it will be a readable compile-time error. I do not find any disadvantages of this solution, except that the universal reference as an output parameter can mislead someone.

Kojoley commented 2 years ago

I do not find any disadvantages of this solution, except that the universal reference as an output parameter can mislead someone.

The disadvantage is to add complexity to support very tiny minority of use cases. For me, it is just not worth of saving from typing something on a separate line.

denzor200 commented 2 years ago

The disadvantage is to add complexity to support very tiny minority of use cases. For me, it is just not worth of saving from typing something on a separate line.

You right, nowadays this issue very tiny minority of use cases. But in the nearest future pfr will be used instead of adapt macro and so this issue will be the most populat case to use x3 with structs(i suppose). Let's save x3's interface as simple as now.

I do remember that you said about automatic detection reflectable structure, without indication from user by "view". But everything has its time.