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: Pass container attribute through sequence #370

Conversation

Projects
None yet
3 participants
@Kojoley
Copy link
Collaborator

Kojoley commented Feb 25, 2018

If underlying parser of sequence is sequence or unary ending down
to a sequence the container attribute should be passed through.

TODO:

  • I have a naming problem, hope to receive a feedback.
  • I don't like SFINAE, but other solutions will have more boilerplate.
    Any suggestions?

Fixes https://svn.boost.org/trac10/ticket/12085

@Kojoley Kojoley referenced this pull request Feb 25, 2018

Closed

Test for bug #12085 #181

@djowel

This comment has been minimized.

Copy link
Member

djowel commented Feb 25, 2018

This is in my radar screen. I don't have a good answer yet, nor a good name. Still thinking about it.

@djowel

This comment has been minimized.

Copy link
Member

djowel commented Feb 26, 2018

Barring a better name, let's have this a go. As for SFINAE alternative, you can probably use overloading and pass in an MPL bool_ to differentiate the two cases?

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

Kojoley commented Feb 26, 2018

As for SFINAE alternative, you can probably use overloading and pass in an MPL bool_ to differentiate the two cases?

It is only one alternative I know and it looks ugly Kojoley@8b76132,

@djowel

This comment has been minimized.

Copy link
Member

djowel commented Feb 26, 2018

I'm OK either way. Implementation details.

@Kojoley Kojoley force-pushed the Kojoley:x3-pass-container-attribute-through-sequence branch 2 times, most recently from 6aa7fb6 to 9365ada Feb 27, 2018

@cppljevans

This comment has been minimized.

Copy link
Contributor

cppljevans commented Feb 28, 2018

With regard to the naming issue, I'd suggest the following renames:

  • parse_into_container_or_proxy -> parse_into_container_or_parser
  • should_sequence_proxy -> should_into_container

Then the calling code would be:

parse_into_container_or_parser(parser.left, first, last, context, rcontext, attr, should_into_container<Parser::left_type, Context>{})

and the called code would be one of:

    template <typename Parser, typename Iterator, typename Context
      , typename RContext, typename Attribute>
    bool parse_into_container_or_parser(
        Parser const& parser
      , Iterator& first, Iterator const& last, Context const& context
      , RContext& rcontext, Attribute& attr, mpl::true_)
    {
        return parser.parse(first, last, context, rcontext, attr);
    }

    template <typename Parser, typename Iterator, typename Context
      , typename RContext, typename Attribute>
    bool parse_into_container_or_parser(
        Parser const& parser
      , Iterator& first, Iterator const& last, Context const& context
      , RContext& rcontext, Attribute& attr, mpl::false_)
    {
        return parse_into_container(parser, first, last, context, rcontext, attr);
    }

This is a better naming because parse_into_container_or_parser, when
separated by the or, gives 2 names:

  • parse_into_container
  • parser

which match the 2 differences in leading string of the called
functions in the bodies of the 2 different overloads:

  • parse_into_container(parser, first, last, context, rcontext, attr);
  • parser.parse(first, last, context, rcontext, attr);

Hmmm... The should_into_container, when true, causes the parser.parse
to be called. Should that be the other way around?

Or, for more encapsulated code, move the specializations and the using
into a separate struct with static calls such as:

    struct parse_into_container_or_parser
    {
    private:
        template <typename Parser, typename Context>
        using which = mpl::bool_<(sequence_size<Parser, Context>::value > 1)>;
      
        template <typename Parser, typename Iterator, typename Context
          , typename RContext, typename Attribute>
        static bool call(
            Parser const& parser
          , Iterator& first, Iterator const& last, Context const& context
          , RContext& rcontext, Attribute& attr, mpl::true_)
        {
            return parser.parse(first, last, context, rcontext, attr);
        }
    
        template <typename Parser, typename Iterator, typename Context
          , typename RContext, typename Attribute>
        static bool call(
            Parser const& parser
          , Iterator& first, Iterator const& last, Context const& context
          , RContext& rcontext, Attribute& attr, mpl::false_)
        {
            return parse_into_container(parser, first, last, context, rcontext, attr);
        }
    public:
        template <typename Parser, typename Iterator, typename Context
          , typename RContext, typename Attribute>
        static bool call(
            Parser const& parser
          , Iterator& first, Iterator const& last, Context const& context
          , RContext& rcontext, Attribute& attr)
        {
            return call(parser, first, last, context, rcontext, attr, which<Parser, Context>{});
        }
    };

    template <typename Parser, typename Iterator, typename Context
      , typename RContext, typename Attribute>
    bool parse_sequence(
        Parser const& parser , Iterator& first, Iterator const& last
      , Context const& context, RContext& rcontext, Attribute& attr
      , traits::container_attribute)
    {
        Iterator save = first;
        if (parse_into_container_or_parser::call(parser.left, first, last, context, rcontext, attr)
            && parse_into_container_or_parser::call(parser.right, first, last, context, rcontext, attr))
            return true;
        first = save;
        return false;
    }

As you can see, the alias template has been renamed to which and privatized
into the new struct parse_into_container_or_parser. Furthermore, both the
specializations have been renamed to call and also privatized into the new
struct. This also makes the calling code cleaner.

The renaming of the alias template to which is meant to suggest a similarity
of the above to variant visitor's. If you think about it, the new parse_into_container_or_parser
a certain similarities to boost's static_visitor:

http://www.boost.org/doc/libs/1_64_0/doc/html/variant/reference.html#variant.concepts.static-visitor.examples

where, instead of the member functions, operator()(...)'s, there's the static call(...)'s.
Maybe that would suggest renaming the new struct to contain visitor in it's name
somewhere to further suggest the similarity with variant's static_visitor. All this is
to somehow make it easier to understand what the code is doing.

Also, it might be a good idea to, within the new struct, supply a short comment
about what problem is solved there. This would also make the code easier to
understand.

OR, simply use an if statement with the compile time value of the "discriminator":

using which = mpl::bool_<(sequence_size<_Parser, Context>::value > 1)>

Since which::value is a compile time value, the compiler, I'd hope, would just
generate code for the branch of the if actually needed. Here's that code:

    template <typename Parser, typename Iterator, typename Context
      , typename RContext, typename Attribute>
    bool parse_sequence(
        Parser const& parser , Iterator& first, Iterator const& last
      , Context const& context, RContext& rcontext, Attribute& attr
      , traits::container_attribute)
    {
        Iterator save = first;
        auto parse_into_container_or_parser=
          [&](auto const& _parser)
          {
              using _Parser=decltype(_parser);
              using which = mpl::bool_<(sequence_size<_Parser, Context>::value > 1)>;
              if(which::value)
              {
                  return parse_into_container(_parser,first,last,context,rcontext,attr);
              }
              else
              {
                  return _parser.parse(first,last,context,rcontext,attr);
              }
          };
        if (parse_into_container_or_parser(parser.left)
            && parse_into_container_or_parser(parser.right))
            return true;
        first = save;
        return false;
    }

This seems simplest and most modular. It's most modular because there's
no external overloaded functions used.

@Kojoley Kojoley force-pushed the Kojoley:x3-pass-container-attribute-through-sequence branch from 9365ada to f3838cd Feb 28, 2018

@Kojoley Kojoley force-pushed the Kojoley:x3-pass-container-attribute-through-sequence branch from f3838cd to fac9dfa Feb 28, 2018

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

Kojoley commented Feb 28, 2018

I like how it looks with lambda, but unfortunately it will not work without constexpr if.

For some reason on VS 2015/2017 the if behaves like it is constexpr even without /std:c++17. GCC and Clang does not have this bugfeature.

@djowel

This comment has been minimized.

Copy link
Member

djowel commented Feb 28, 2018

constexpr if can solve a lot of the verbosity indeed. should we us it or not is another question.

@cppljevans

This comment has been minimized.

Copy link
Contributor

cppljevans commented Mar 1, 2018

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

Kojoley commented Mar 1, 2018

The gist shows that the constexpr if lambda method doesn't work as expected :(

You forgot to strip const ref.

@Kojoley Kojoley merged commit a0df3c0 into boostorg:develop Mar 1, 2018

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-pass-container-attribute-through-sequence branch Mar 1, 2018

@djowel

This comment has been minimized.

Copy link
Member

djowel commented Mar 1, 2018

Let's keep this in line with what the current implementation does (either sfinae or static bool_). When constexpr if is more prevalent, then we can use it.

@cppljevans

This comment has been minimized.

Copy link
Contributor

cppljevans commented Mar 2, 2018

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

Kojoley commented Mar 2, 2018

But doesn't the Merged#370 code use sfinae.

Yes it is a sfinae, I've even written about it in the PR description.

However, the Merged#370 code is not as modular as that here:
So, wouldn't the more modular #file-parse_sequence_container-cpp-L95 code be preferable?
OTOH, the current implementation is not as modular ;)

I do not understand what are you meaning under 'modular' here. If you meant customization point -- it is not needed here, and can't be here as it is in detail namespace.

OTOH, the current implementation is not as modular ;)

Current implementation is less code, it does the same thing as yours (not less, not more). Compilation speed is the same in the all cases:

MSVC 14.1 (15.5.7)

~ compilation timings
before pr 5 loops, best of 3: 3.14 sec per loop
after pr 5 loops, best of 3: 3.15 sec per loop
"modular" 5 loops, best of 3: 3.15 sec per loop
Benchmark code
#include <boost/spirit/home/x3.hpp>
#include <boost/fusion/include/vector.hpp>
#include <string>
#include <vector>

namespace x3 = boost::spirit::x3;

int main()
{
    boost::fusion::vector<
        std::vector<int>, std::vector<std::string>
      , std::vector<int>, std::vector<std::string>
      , std::vector<int>, std::vector<std::string>
      , std::vector<int>, std::vector<std::string>
      , std::vector<int>, std::vector<std::string>
    > attr;

    char *iter = nullptr, *last = nullptr;
    phrase_parse(iter, last,
           "ints: " >> (x3::int_ % ',') >> "strings: " >> *x3::lexeme['[' >> +~x3::char_(']') >> ']']
        >> "ints: " >> (x3::int_ % ',') >> "strings: " >> *x3::lexeme['[' >> +~x3::char_(']') >> ']']
        >> "ints: " >> (x3::int_ % ',') >> "strings: " >> *x3::lexeme['[' >> +~x3::char_(']') >> ']']
        >> "ints: " >> (x3::int_ % ',') >> "strings: " >> *x3::lexeme['[' >> +~x3::char_(']') >> ']']
        >> "ints: " >> (x3::int_ % ',') >> "strings: " >> *x3::lexeme['[' >> +~x3::char_(']') >> ']']
      , x3::space, attr);

    return 0;
}
@cppljevans

This comment has been minimized.

Copy link
Contributor

cppljevans commented Mar 2, 2018

@djowel

This comment has been minimized.

Copy link
Member

djowel commented Mar 2, 2018

I really appreciate your concern and effort, @cppljevans. However, there are certain coding idioms that run across the current code base. Understanding the common idioms will give you a good understanding of how the whole library works. I am opposed to a code base using a multitude of idioms when one will do. That will make the code a lot more difficult to read and understand. Right now, the idiom is the use of static bool_ and overloading (and some, but lesser of sfinae actually). There can be a better idiom, sure. But whatever it is, it should be generally applicable to all code and does not rely on some compiler quirks. constexpr if is a good candidate, but until that one becomes more prevalent and practical to use across c++ compilers, I do not think we want to switch yet.

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.