boostorg / spirit

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

X3: Rework attribute for alternative parser #740

Open eido79 opened 1 year ago

eido79 commented 1 year ago

Implement the following behavior, as described in #610

eido79 commented 1 year ago

Hi @Kojoley,

I haven't made deep inspection of what could go wrong with doing variant<unused_type, ...>, have you? At least the question about how crude is unused_type in visitor will be, since it has implicit constructor from everything.

Not really, admittedly. The fact that unused_type is constructible from everything might indeed create problems. I'll do some more analysis and try to come up with some meaningful use cases.

In case this turn out to be ugly, it is anyway easy to make the behavior of attribute_of_alternative match the currently documented rules, i.e. use optional<variant<...>> when unused_type is involved.

A trait to configure monostate type is needed.

Agreed. How do you think that should look like?

No need to use boost type traits which are available in standard library. Put the tests in alternative.cpp/sequence.cpp please.

OK.

Kojoley commented 1 year ago

I haven't made deep inspection of what could go wrong with doing variant<unused_type, ...>, have you? At least the question about how crude is unused_type in visitor will be, since it has implicit constructor from everything.

Not really, admittedly. The fact that unused_type is constructible from everything might indeed create problems. I'll do some more analysis and try to come up with some meaningful use cases.

In case this turn out to be ugly, it is anyway easy to make the behavior of attribute_of_alternative match the currently documented rules, i.e. use optional<variant<...>> when unused_type is involved.

I am afraid optional<variant<...>> might actually be not supported currently by alternative parser/is_subsitute, I couldn't find tests for it either.

A trait to configure monostate type is needed.

Agreed. How do you think that should look like?

Something like https://github.com/boostorg/spirit/blob/develop/include/boost/spirit/home/x3/support/traits/optional_traits.hpp

template <typename Variant> struct variant_monostate { using type = unused_type; };
eido79 commented 1 year ago

It's indeed ugly. Since anything is convertible to unused_type, things like this compile just fine

char input[] = "Whoops";
boost::variant<x3::unused_type, int> v;
auto first = std::begin(input);
auto last = std::end(input) - 1;
bool success = x3::parse(first, last, x3::int_ | +x3::char_, v);
assert(success && first == last); // OK!

hiding potential mistakes that should be caught at compile time.

At this point, I'm thinking that using boost::blank or another empty type would be a better choice. Alternatively, it could be investigated what it would take to make optional<variant<...>> work properly.

Thoughts?