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: Removed sequence into plain parsing #439

Merged
merged 2 commits into from Jan 21, 2019

Conversation

Projects
None yet
3 participants
@Kojoley
Copy link
Collaborator

commented Jan 15, 2019

Closes #438.

@djowel

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

I assume all tests pass? Just be sure this does not affect rules. I think you are in the right direction, but I'm somewhat worried.

@cppljevans
Copy link
Contributor

left a comment

pass_sequence_container_attribute should be renamed to disable_into_container since,
judging from:

template <typename Parser, typename Iterator, typename Context
, typename RContext, typename Attribute>
typename enable_if_c<pass_sequence_container_attribute<Parser, Context>, bool>::type
parse_sequence_container(
Parser const& parser
, Iterator& first, Iterator const& last, Context const& context
, RContext& rcontext, Attribute& attr)
{
return parser.parse(first, last, context, rcontext, attr);
}
template <typename Parser, typename Iterator, typename Context
, typename RContext, typename Attribute>
typename disable_if_c<pass_sequence_container_attribute<Parser, Context>, bool>::type
parse_sequence_container(
Parser const& parser
, Iterator& first, Iterator const& last, Context const& context
, RContext& rcontext, Attribute& attr)
{
return parse_into_container(parser, first, last, context, rcontext, attr);
}

that name more accurately expresses its meaning.

Yes, that's more changes without any semantics difference; however,
it improves the readability of the code.

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 16, 2019

I assume all tests pass? I think you are in the right direction, but I'm somewhat worried.

Yes, and examples compile too. We also have CI set up for this purpose.

Just be sure this does not affect rules.

The commit where the code come 235359a seems to be about tuple elements pass-through, and rules itself is just a parser for sequences.

I think you are in the right direction, but I'm somewhat worried.

I expect this change to break the code that is already silently broken (like #434).

@cppljevans I prefer not to make unrelated changes. I agree that my naming may be confusing, you can open a PR for this, but the pass means something like pass-through or forwarding, it was named after the pass_sequence_attribute trait.

@cppljevans
Copy link
Contributor

left a comment

Shouldn't there be a test verifying the statement:

The eps >> eps parser will still parse into tuple<>.

?

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 17, 2019

I would like to add a test case that ensures eps >> eps does NOT compile with tuple<> attribute, but I do not want to make the current code more complex just to disable it and compile fail tests are too slow (end requires a separate file for every case).

@cppljevans

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

I would like to add a test case that ensures eps >> eps does NOT compile with tuple<> attribute, but I do not want to make the current code more complex just to disable it and compile fail tests are too slow (end requires a separate file for every case).

If it does NOT compile, then I'm puzzled because I'm getting it to compile. With:

#include <boost/spirit/home/x3.hpp>
#include <boost/fusion/container/vector.hpp>
int main()
{
    {
        using boost::spirit::x3::eps;
    //Copy&past from:
    //  https://github.com/boostorg/spirit/pull/439/commits/2235da2416b1ffbaa167aa2e9ebc5aeb98eab11b
        char const* const s = "";
        boost::fusion::vector<> a;
        parse(s, s, eps >> eps, a);
    }
    return 0;
}

It compiles with your patch:

/usr/bin/clang++ -c -O0 -gdwarf-2  -std=c++17 -ftemplate-backtrace-limit=0 -fdiagnostics-show-template-tree -fno-elide-type -fmacro-backtrace-limit=0     -I../../x3-Removed_sequence_into_plain_parsing-zip/include -I/home/evansl/prog_dev/boost/releases/ro/boost_1_69_0    -ftemplate-depth=200  test-sequence-eps2.cpp -MMD -o /tmp/build/clangxx6_0_pkg/boost/releases/ro/boost_1_69_0/sandbox/kojoley/test/x3-Removed_sequence_into_plain_parsing/test-sequence-eps2.o 
/usr/bin/clang++    /tmp/build/clangxx6_0_pkg/boost/releases/ro/boost_1_69_0/sandbox/kojoley/test/x3-Removed_sequence_into_plain_parsing/test-sequence-eps2.o   -o /tmp/build/clangxx6_0_pkg/boost/releases/ro/boost_1_69_0/sandbox/kojoley/test/x3-Removed_sequence_into_plain_parsing/test-sequence-eps2.exe
/tmp/build/clangxx6_0_pkg/boost/releases/ro/boost_1_69_0/sandbox/kojoley/test/x3-Removed_sequence_into_plain_parsing/test-sequence-eps2.exe 

Compilation finished at Thu Jan 17 07:41:17

Your patch is in the:

../../x3-Removed_sequence_into_plain_parsing-zip/include

directory.

The -zip directory was downloaded early yesterday from:

https://github.com/boostorg/spirit/tree/a6132ae361b5f4d3480d23200ea8d10b8911ff55

What might I be doing wrong?

TIA.

-regards,
Larry

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 17, 2019

What might I be doing wrong?

Possibly I was not clear enough, I try to put emphasis on my previous message

I would like to add a test case that ensures eps >> eps does NOT compile with tuple<> attribute, but I do not want to make the current code more complex just to disable it ...

I consider it as a bug and it is out of this PR scope. I have created #440 about it.

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 20, 2019

I think I figured out why sequence handling was done like this. The current fix was impossible without a8e391b/#340, before it sequence would try to unwrap its sequence-iterator even if there is a nested sequence and if unwrap was done the nested sequence receives a plain attribute which was handled by parse_sequence_plain. After a8e391b/#340 a sequence-iterator will always pass-through to nested sequences and unwrapped attribute never gets into the sequence parser, so parse_sequence_plain became obsolete.

@djowel

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

Wonderful!

Kojoley added some commits Jan 15, 2019

X3: pass_sequence_attribute: Removed unused specialization
The `partition_attribute` never splits a sequence into zero sized parts.

Removing the specialization simplifies an ongoing patch that will forbid
sequence into plain types parsing.
X3: Removed sequence into plain parsing
The possibility to parse `int_ >> int_` into `int` is dangerous and hides the
actual bugs in user parsers (and in Spirit too).

@Kojoley Kojoley force-pushed the Kojoley:x3-remove-sequence-to-plain branch from a6132ae to d80d32f Jan 21, 2019

@Kojoley Kojoley merged commit ab28052 into boostorg:develop Jan 21, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Kojoley Kojoley deleted the Kojoley:x3-remove-sequence-to-plain branch Jan 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.