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

X3: Rework attribute for alternative parser #740

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

eido79
Copy link

@eido79 eido79 commented Oct 30, 2022

Implement the following behavior, as described in #610

  • Remove duplicates
  • Simplify variant<X> to X
  • Make unused_type the first type of the variant
  • Transform variant<unused_type, T> to optional<T>

Copy link
Collaborator

@Kojoley Kojoley left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. A few things need to be resolved:

  1. 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.
  2. A trait to configure monostate type is needed.
  3. No need to use boost type traits which are available in standard library.
  4. Put the tests in alternative.cpp/sequence.cpp please.

@eido79 eido79 changed the title Rework attribute for alternative parser X3: Rework attribute for alternative parser Nov 1, 2022
@eido79
Copy link
Author

eido79 commented Nov 1, 2022

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
Copy link
Collaborator

Kojoley commented Nov 2, 2022

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
Copy link
Author

eido79 commented Nov 2, 2022

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?

Implement the following rules
* Remove duplicates
* Simplify variant<X> to X
* Make unused_type the first type of the variant
* Transform variant<unused_type, T> to optional<T>
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.

None yet

2 participants