Added readBison() #54
Conversation
There was a problem hiding this comment.
I don't see any obvious issues in the parsertl code. Could you please check the other comments, though?
Especially the approach with regard to the static vs. object method is IMHO something that should be looked at more closely. I could further tall for static for example, some factory like API could be initiated. For instance there could be another method to return a parser object like Parser::fromBisonFile("/path/to/file").
Thanks
tests/readBison.phpt
Outdated
| $p = new Parser; | ||
|
|
||
| $p->readBison("%%\n;start: 'a';%%\n"); | ||
| $p->build(); |
There was a problem hiding this comment.
Would it be expected, that any rules to be possibly added after readBison() call? If not, perhaps API wise it would be cleaner to have a static method that would look like
// Create and return the full object which is already in the finalized state
$p = Parser::fromBison(...);Even if it should be possible to add more rules after reading the dump, perhaps semantically still it would make sense to have it in the static variant to save the line with the new call?
Thanks
There was a problem hiding this comment.
Right now, it is simply an alternative to calling all the different functions on a rule object.
tests/readBison.phpt
Outdated
| $lex->push("a", $p->tokenId("'a'")); | ||
| $lex->build(); | ||
|
|
||
| echo $p->validate("a", $lex) . "\n"; |
There was a problem hiding this comment.
Could you switch to using var_dump instead? It'll show the type of the returned value as well.
Thanks
There was a problem hiding this comment.
The test now does a dump() instead and the test also supports UTF-32
| @@ -0,0 +1,24 @@ | |||
| --TEST-- | |||
| readBison() test | |||
There was a problem hiding this comment.
Could we have yet a test where an exception is thrown? Like perhaps feed some random garbage, etc. or something that would certainly fail it?
Thanks
|
Ben, could you please also add like |
|
|
||
| parsertl::debug::dump(par.rules, ss); | ||
| str = ss.str(); | ||
| php_write((void*)str.c_str(), str.size()); | ||
| #endif |
There was a problem hiding this comment.
This seems to be fixing #27, not related to the readBison feature. Should it be separated into another PR please?
Thanks
There was a problem hiding this comment.
This fix is necessary in order for the readBison() test to work.
Thanks
There was a problem hiding this comment.
I see. Perhaps it just should have been approached in reverse order - first the dump fix, then the bison fix. Then one could split features by PR. Looks like it'd be best to land both in this PR then, to avoid additional inconvenience. Probably should land this one once both features are finalized then. I'll check to add more tests also once this is in.
Thanks
There was a problem hiding this comment.
Can we merge this one now?
Like you said, we can add extras later.
I'd like to address the other issues ASAP now. I'm hoping they will all be quick from now on.
I'd like to address the other std::cout calls next.
There was a problem hiding this comment.
Yeah, it looks fine now. I think the last small piece is to add the missing header to the package.xml to make all green, then can continue on the fresh state.
Thanks
| template<typename T> | ||
| void stream_num(const T num_, std::basic_ostream<char32_t>& ss_) | ||
| { | ||
| std::stringstream css_; |
There was a problem hiding this comment.
I guess the build complains about missing sstream include here?
Thanks
There was a problem hiding this comment.
Thanks for the heads up. Fixed now.
Fixes: #29