Closed gck-ableton closed 8 years ago
@gck-ableton: This sounds awesome! I just took a quick look and didn't delve into the grammar. At that level of detail this all looks reasonable. Just one question: If I read it correctly parseStdString
rebuilds the grammar every time it is called, correct? If so, did you consider, building it once? I don't know how expensive it is though.
Awesome! 🎉 Considering that I didn't review the original parser, my first impulse would be to just believe you that the thing works fine if all tests are passing, but I might take a look anyway.
If you are going to review the grammar changes you will find, that they are not 100% identical. There has been some ambiguities in the boost::spirit one, which boost resolved in a different way than peglib does. And since the new grammar is a PEG one, we have to care about ignored whitespaces (ign(WS)
) in a more explicit way than before - which (though slightly less readable) is good, I think.
@mli-ableton yes, the grammar is rebuild every time currently. This is because the actions capture local variables. There's more room for optimization though, because the current actions use peglib's any
type, which works like boost::variant
, and are heap based. I'll experiment with a different way of translating the parsed data into our structure, which might save most of that overhead. And the expected impact of that is likely larger then using a single grammar for all parsing. But all of that I would like to do later and not on this branch.
Besides dropping the massive dependency on boost::spirit (and all its required dependencies) this
@mli-ableton @mak-ableton Not sure whether you want to take a look.