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

optional sequence returns #67

Closed
akrzemi1 opened this issue Jan 15, 2024 · 8 comments
Closed

optional sequence returns #67

akrzemi1 opened this issue Jan 15, 2024 · 8 comments

Comments

@akrzemi1
Copy link
Contributor

Going back to the zero-or-more-element separated sequence, the workaround with attr proposed in #60 doesn't work in more complex cases. The following, a bit unfair, example compares the Boost.Spirit solution with -(p % s) and Boost.Parser solution with (p % s) | attr(A{}), wehre p parses a tuple.

The problem is that when I use my aggregate type A, then attribute variant<tuple<M1, M2>, A> is not merged to just A (or tuple<M1, M2>). In other words, A and tuple<M1, M2> are not treated as "same" or "equivalent".

Maybe this could be changed? If not, I ask you to reconsider #63. Or maybe there is a way to define one's own directive? I find the use of a rule -- involving the use of macros and multiple definitions -- too big a solution for so common a problem.

#ifdef WITH_BOOST_SPIRIT
#include <boost/spirit/include/qi.hpp>
namespace bp = boost::spirit::qi;
using boost::optional;
#else
#include <boost/parser/parser.hpp>
namespace bp = boost::parser;
using std::optional;
#endif
#include <boost/fusion/adapted/struct/adapt_struct.hpp>

struct P
{
  char c;
  int i;
};
BOOST_FUSION_ADAPT_STRUCT(P, c, i);

int main()
{
#ifdef WITH_BOOST_SPIRIT
  auto alnum = bp::ascii::alnum;
  auto alpha = bp::alpha;
#else
  auto alnum = bp::ascii::alnum;
  auto alpha = bp::ascii::alpha;
#endif

#ifdef WITH_BOOST_SPIRIT
  auto myParser = -((bp::char_ >> bp::int_) % ',');
#else
  auto myParser = ((bp::char_ >> bp::int_) % ',') | bp::attr(std::vector<P>{});
#endif

  std::string input = "1, 2";
  std::vector<int> result;

#ifdef WITH_BOOST_SPIRIT
  auto begin = input.begin();
  bool b = bp::phrase_parse(begin, input.end(), myParser, bp::ascii::space, result);
#else
  auto b = bp::parse(input, myParser, bp::ws, result);
#endif

  if (b)
    std::cout << ":-) " << std::endl;
  else
    std::cout << ":-( " << std::endl;
}
@tzlaine
Copy link
Owner

tzlaine commented Jan 15, 2024

Yeah, that doesn't work, but again, that's what rules are for. You can create a rule like:

bp::rule<struct ps_tag, std::vector<P>> ps = "ps";
auto ps_def = (bp::char_ >> bp::int_) % ',') | bp::eps;

And that will work fine I think. Rules exist explicitly for this purpose -- to fix the attribute type to something other than the default.

@tzlaine tzlaine closed this as completed Jan 15, 2024
@akrzemi1
Copy link
Contributor Author

akrzemi1 commented Jan 16, 2024

According to the docs, that would actually be three lines:

bp::rule<struct ps_tag, std::vector<P>> ps = "ps";
auto ps_def = ((bp::char_ >> bp::int_) % ',') | bp::eps;
BOOST_PARSER_DEFINE_RULE(ps);

And I find it quite problematic. My whole parser looks like this:
https://github.com/akrzemi1/wdungeon/blob/master/parser.cpp#L33
It is 15 lines, each defining a parser. I have no recursion. This layout has a nice property -- this is the huge advantage of Boost.Spirit -- that the code looks almost as if I were defining the EBNF description.

This will now break when I need to inject the rule definitions. And I have two of them.

BTW, is there a way to define a generic rule? That is, can I specify a rule that applies my pattern (parse p zero or more times, comma separated) but allow me to instantiate it for different parsers p?

@tzlaine
Copy link
Owner

tzlaine commented Jan 17, 2024

According to the docs, that would actually be three lines:

bp::rule<struct ps_tag, std::vector<P>> ps = "ps";
auto ps_def = ((bp::char_ >> bp::int_) % ',') | bp::eps;
BOOST_PARSER_DEFINE_RULE(ps);

And I find it quite problematic. My whole parser looks like this: https://github.com/akrzemi1/wdungeon/blob/master/parser.cpp#L33 It is 15 lines, each defining a parser. I have no recursion. This layout has a nice property -- this is the huge advantage of Boost.Spirit -- that the code looks almost as if I were defining the EBNF description.

This will now break when I need to inject the rule definitions. And I have two of them.

I told you earlier the problems I have with the attribute combination logic in Spirit. I don't really think making your example stick to 15 lines instead of 20 is a very strong counter-argument. Also, the Boost.Parser version could lose 29 lines of ADAPTed_STRUCTs.

BTW, is there a way to define a generic rule? That is, can I specify a rule that applies my pattern (parse p zero or more times, comma separated) but allow me to instantiate it for different parsers p?

Well, a rule is just a variable, and you can make variable templates. The fact that the tag type needs to depend on the template parameter is a small wrinkle, but something like this:

template<typename P>
struct my_rule_tag {};

template<typename P, typename Attr>
bp::rule<my_rule_tag<P>, std::vector<Attr>> my_rule = "my_rule";
template<typename P, typename Attr>
constexpr auto my_rule_def = (P{} % ',') | bp::attr(std::vector<Attr>{});

... might work, but you'll have to write out the function overloads that would normally be generated by BOOST_PARSER_DEFINE_RULES; they'll have to gain a template parameter T. You could also just make a non-rule variable template for parsers that already have the right attribute, like:

template<typename P>
constexpr auto no_attr_repeated_p = omit[*p];

@akrzemi1
Copy link
Contributor Author

I acknowledge your goal of having simple attribute merge rules. I am therefore exploring other options. Parametrized rule may not be possible. But a directive looks like something that is already parametrized by the parser.

Is it possible to provide a tool for defining custom directives?

@tzlaine
Copy link
Owner

tzlaine commented Jan 18, 2024

Maybe? This is just not something that comes up very often. In fact, I've never needed such functionality in years and years of using Spirit to write dozens of parsers. However, I think if I had a compelling use case, I could be persuaded.

@akrzemi1
Copy link
Contributor Author

Ok, so the regression of Boost.Parser relative to Boost.Spirit is that for the parser that needs to parse "zero-or-more occurrences of comma-separated items", I no longer can define it at function or block scope. The only way for me is to use a rule, but a rule can only be defined at namespace scope.

In my program, as long as I were using Boost.Spirit, i was able to encapsulate my parser in a single function body.
https://github.com/akrzemi1/wdungeon/blob/master/parser.cpp#L33
Now I will have to move my code outsde the function body. Is that correct?

@tzlaine
Copy link
Owner

tzlaine commented Jan 21, 2024

Yes, that's correct, except for the characterization of "regression". It's more like choosing a slightly different set of trade-offs.

Some things to note, though. Rules and parsers are constant data. Are you really that sensitive to where you define your constants? I tend to have lots of namespace-scope contants, and it never caused me any aggravation. I care about where code is when it affects my ability to do local reasoning. With code that describes constants... not so much.

Also, so far you've been treating rules as this weird thing that you have to put up with. Most people who use Spirit (and later Parser) have, and will continue to, use rules all the time. I encourage you to embrace them as well. Except for the smallest parsers, rules are essential for organizing things, and writing good tests in particular.

@akrzemi1
Copy link
Contributor Author

My reservation is not towards rules in general, but towards their interface in Boost.Parser. I will describe it in a separate issue.

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

No branches or pull requests

2 participants