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

[WIP] experimental Spirit X3 hacks #3976

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lightmare
Copy link
Contributor

This is an experiment I started because I hate how Spirit X3 separates rules from parsing expressions with the _def suffix. Jumping between where a rule is used in the grammar and where it's defined is awkward because they're different identifiers. Well, I could make vim do that with a keystroke, but I chose not to write vimscript for this.

C++ allows a name to refer to a type and a variable (or function) at the same time. They don't shadow each other. Although the variable/function takes precedence where it's syntactically allowed, you can always get to the type with an explicit class-key (struct, enum, ...). So I use the same name for the Tag type used to differentiate x3::rule specializations, and for the variable used in parsing expressions.

Rule parsing expressions are stored in a variable template specialized on the Tag type. After testing a few variations of this approach, I found it's already been explored (djowel/spirit_x3#17).

So there are no suffixes, everything related to a rule is designated by a single name.

struct MyRule { static MyRule proxy; ... }; // type named MyRule
struct MyRule const MyRule = MyRule::proxy; // variable named MyRule
MyRule MyRule::proxy = "Some nonterminal";
template <> auto const _rule_def_<MyRule> = ... parsing expression ...

The MyRule variable could've been simply an x3::rule instance, but that would suffer from initialization order issues because x3::rule constructor is not constexpr. So I instead made the struct MyRule type usable in parsing expressions (it converts to an x3::rule).

So far I've only converted TopoJSON grammar to this style (despite my opinion that Spirit is not the right tool to parse JSON-based protocols and that this grammar is flawed, I had my hands on it when I got the idea :)). With gcc-6, topojson_grammar_x3.o size dropped by ~30k (~10%), that accounts for ~1% of libmapnik-json.a. I'm curious whether other grammars will also shrink. I kinda expected the opposite, as I made x3::rule type signatures longer. Perhaps the reduction is due to BOOST_SPIRIT_DEFINE putting parsing expressions in function-local static variables, whose names are really long, because they include both the function type and the variable type.

@lightmare
Copy link
Contributor Author

I'll try to explain how this works on a simple grammar. Let's parse a subset of JSON that only knows booleans, numbers, and arrays.

Rules that are not involved in recursive cycles, and don't need to be used from other sources, are defined with MAPNIK_SPIRIT_LOCAL_RULE(Tag[, Attribute]). The Attribute argument is optional and defaults to x3::unused_type.

// grammar_def.hpp

MAPNIK_SPIRIT_LOCAL_RULE(r_boolean, bool)
    = "false" >> x3::attr(false)
    | "true" >> x3::attr(true)
    ;

When there is a recursive cycle, some rule(s) must be introduced first with MAPNIK_SPIRIT_RULE(Tag[, Attribute]) and later specialized with MAPNIK_SPIRIT_RULE_DEF(Tag).

// grammar_def.hpp

MAPNIK_SPIRIT_RULE(r_value, my_value_type);

MAPNIK_SPIRIT_LOCAL_RULE(r_array, my_array_type)
    = '[' > -(r_value % ',') > ']'
    ;

MAPNIK_SPIRIT_RULE_DEF(r_value)
    = x3::double_
    | r_boolean
    | r_array
    ;

When a rule is to be used from other grammars/sources, it goes like this:

// grammar.hpp

MAPNIK_SPIRIT_EXTERN_RULE(r_start_rule, boost::optional<my_value_type>);

// grammar_def.hpp

MAPNIK_SPIRIT_EXTERN_RULE_DEF(r_start_rule)
    = r_value
    | "null"
    ;

// grammar.cpp

MAPNIK_SPIRIT_INSTANTIATE(r_start_rule, iterator_type, context_type);

And finally, all rules must have their fancy names (those that appear in x3::expectation_failure exception) defined once:

// grammar.cpp

MAPNIK_SPIRIT_RULE_NAME(r_boolean) = "Simpleson boolean";
MAPNIK_SPIRIT_RULE_NAME(r_value) = "Simpleson value";
MAPNIK_SPIRIT_RULE_NAME(r_array) = "Simpleson array";
MAPNIK_SPIRIT_RULE_NAME(r_start_rule) = "Simpleson message";

@codecov
Copy link

codecov bot commented Aug 24, 2018

Codecov Report

Merging #3976 into master will increase coverage by 0.02%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3976      +/-   ##
==========================================
+ Coverage   71.44%   71.47%   +0.02%     
==========================================
  Files         439      441       +2     
  Lines       22885    22837      -48     
==========================================
- Hits        16351    16322      -29     
+ Misses       6534     6515      -19
Impacted Files Coverage Δ
include/mapnik/json/topology.hpp 0% <ø> (ø) ⬆️
include/mapnik/json/topojson_utils.hpp 0% <0%> (ø) ⬆️
include/mapnik/util/spirit_rule.hpp 100% <100%> (ø)
include/mapnik/json/topojson_grammar_x3.hpp 50% <50%> (ø)
include/mapnik/feature_type_style.hpp 0% <0%> (-100%) ⬇️
include/mapnik/rule.hpp 0% <0%> (-100%) ⬇️
include/mapnik/text/rotation.hpp 0% <0%> (-100%) ⬇️
include/mapnik/group/group_rule.hpp 0% <0%> (-100%) ⬇️
include/mapnik/value/types.hpp 0% <0%> (-100%) ⬇️
include/mapnik/attribute.hpp 0% <0%> (-66.67%) ⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4bce33...e4d7735. Read the comment docs.

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

1 participant