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

Implement nom::sequence::Tuple for () (Unit) #1526

Merged
merged 1 commit into from May 21, 2022
Merged

Implement nom::sequence::Tuple for () (Unit) #1526

merged 1 commit into from May 21, 2022

Conversation

LoganDark
Copy link
Contributor

Right now, () (Unit) does not implement the nom::sequence::Tuple trait required to use the tuple combinator. This makes it difficult to use in macros.

Example

I'm currently mass-producing parsers using a macro for a fictional instruction set assembler. Each instruction may take a variable number of arguments so I am using this macro component to parse all arguments:

		tuple(($(
			preceded(space1, $parser)),*
		))

This works fine for any number of arguments >=1 and <=21, but notably does not work for a number ==0. Here's why:

the trait bound `(): nom::sequence::Tuple<_, _, _>` is not satisfied

Looks like () does not implement Tuple, even though it could concievably implement a tuple of zero elements.

This pull request implements Tuple for () such that it will always succeed and it is always a no-op. It also adds a test case that verifies that tuple(()) never fails and always returns the input unchanged.

I am unsure if the ParseError<I> bound is needed on the impl, but I figured it's better to be safe than sorry and stay consistent with the existing tuple impls. I would also appreciate assistance with removing the ugly turbofishes from the test.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 79.568% when pulling 476f503 on LoganDark:unit-tuple into 294ffb3 on Geal:main.

@Geal Geal merged commit 761ab0a into rust-bakery:main May 21, 2022
@Geal
Copy link
Collaborator

Geal commented May 21, 2022

good idea, thanks

@LoganDark
Copy link
Contributor Author

It looks like the way you merged the PR clobbered my GPG signature, so it's showing as unverified in the commit log. Unfortunate :/ although my original commit from this PR is signed, so it's not a huge deal.

@Geal
Copy link
Collaborator

Geal commented May 21, 2022

Right, I generally use the "squash and merge" to make one commit per PR and link it to the PR in the title

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

3 participants