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

ForwardIterator -> ReadableIteratorConcept & ForwardTraversalConcept #320

Merged
merged 1 commit into from
Dec 11, 2017

Conversation

wanghan02
Copy link
Contributor

@wanghan02 wanghan02 commented Dec 7, 2017

The concept of ForwardIterator is flawed because it mixed 2 sets of concepts (value access and traversal) into 1 package.

http://www.boost.org/doc/libs/1_65_1/libs/iterator/doc/new-iter-concepts.html

It requires value_type (const)& as return type when dereference is applied, which is not mandatory in spirit parsing. A return type which is convertible to value_type is good enough. ReadableIteratorConcept and ForwardTraversalConcept should be what we need for the iterator check.

For example, the iterator of the range returned by boost::adaptors::transform(std::string, func) is normally not a ForwardIterator. But it fulfills ReadableIteratorConcept and ForwardTraversalConcept and should be able to be parsed by spirit.

Test cases are added for both qi and x3.

There is also a ticket for this but the solution suggested is not correct.
https://svn.boost.org/trac10/ticket/12817

@wanghan02
Copy link
Contributor Author

wanghan02 commented Dec 7, 2017

@Kojoley pull request #230 was not a correct solution (same as in the ticket). But I cannot close that pull request.

The concept of ForwardIterator is flawed because it mixed 2 sets of concepts (value access and traversal) into 1 package.

http://www.boost.org/doc/libs/1_65_1/libs/iterator/doc/new-iter-concepts.html

It requires value_type (const)& as return type when dereference is applied, which is not mandatory in spirit parsing. A return type which is convertible to value_type is good enough. ReadableIteratorConcept and ForwardTraversalConcept should be what we need for the iterator check.

For example, the iterator of the range returned by boost::adaptors::transform(std::string, func) is normally not a ForwardIterator. But it fulfills ReadableIteratorConcept and ForwardTraversalConcept and should be able to be parsed by spirit.
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.

Seems perfectly logical to me.
@djowel I'm going to merge this in a day or two if you have nothing against.

@djowel
Copy link
Member

djowel commented Dec 10, 2017

Hmmm, seems my previous comment did not get through? Or is this a different PR?

@Kojoley
Copy link
Collaborator

Kojoley commented Dec 10, 2017

Who knows.

@djowel
Copy link
Member

djowel commented Dec 10, 2017

OK... anyway, what I wrote was: I like the new iterator traits, but it is not really "new". It was written in 2003 and so far, I don't see it assimilated by the standards (C++11 and onwards). Correct me if I am wrong. So, unless there's real activity towards that, this falls into the category of "don't fix it if it ain't broken".

@Kojoley
Copy link
Collaborator

Kojoley commented Dec 10, 2017

The PR is not about simply switching to "new" iterator concepts, but to make spirit less paranoid and allow the things like boost::transformed_range. Currently it produces the compilation error because of asserts and will work fine if you remove them.

@djowel
Copy link
Member

djowel commented Dec 11, 2017

I see that now. OK, we need this alright. It fixes something that's broken. I'll merge this one.

@djowel djowel merged commit b10e4a4 into boostorg:develop Dec 11, 2017
@djowel
Copy link
Member

djowel commented Dec 11, 2017

And thanks @wanghan02 and @Kojoley

@wanghan02 wanghan02 deleted the thinkcell_iterator_check branch December 11, 2017 09:24
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