Closed GoogleCodeExporter closed 9 years ago
Here are my comments to sidharth's review:
1-3 (DFA, Lexer, Parser):
The type annotation and extra nulled parameters in Parser.php and Lexer.php were
needed to stay PHP strict:
if you'll remove these nulled parameters, and try to run the generated
parser/lexer with
ini_set('error_reporting', E_ALL | E_STRICT);
you'll get warnings like these.
PHP Strict Standards: Declaration of AntlrLexer::matchAny() should be
compatible
with that of BaseRecognizer::matchAny() in
/Users/roll/Documents/projects/antlr-php-runtime/runtime/Php/Lexer.php on line
297
PHP Strict Standards: Declaration of AntlrLexer::recover() should be
compatible with
that of BaseRecognizer::recover() in
/Users/roll/Documents/projects/antlr-php-runtime/runtime/Php/Lexer.php on line
297
PHP Strict Standards: Declaration of AntlrLexer::traceIn() should be
compatible with
that of BaseRecognizer::traceIn() in
/Users/roll/Documents/projects/antlr-php-runtime/runtime/Php/Lexer.php on line
297
PHP Strict Standards: Declaration of AntlrLexer::traceOut() should be
compatible
with that of BaseRecognizer::traceOut() in
/Users/roll/Documents/projects/antlr-php-runtime/runtime/Php/Lexer.php on line
297
PHP Strict Standards: Declaration of AntlrParser::traceIn() should be
compatible
with that of BaseRecognizer::traceIn() in
/Users/roll/Documents/projects/antlr-php-runtime/runtime/Php/Parser.php on line
97
PHP Strict Standards: Declaration of AntlrParser::traceOut() should be
compatible
with that of BaseRecognizer::traceOut() in
/Users/roll/Documents/projects/antlr-php-runtime/runtime/Php/Parser.php on line
97
PHP Strict Standards: Declaration of
Erfurt_Sparql_Sparql10Lexer_DFA35::specialStateTransition() should be
compatible with
that of DFA::specialStateTransition() in
/Applications/MAMP/htdocs/rollxx-sparql2-parser-new/erfurt/src/Erfurt/Sparql/Spa
rql10Lexer.php
on line 1747
it was a quick fix. If there is another way to change this, please tell me.
4 (UnwantedTokenException): thanks, changed committed to github.
5 (TreeRuleReturnScope): looks like I got a strange case where it was needed,
but now
everything woks without it. no need to merge.
6 (TestDFA): I assumed this test is equivalent to DFATest, and because I
changed the
DFA::unpackEncodedString to DFA::unpackRLE, I thought i will test it only once.
7 (Php.stg): I abandoned the support for multiple return values, because I
couldn't
get it to work correctly and found another solution for my problem. I will
probably
get to it later, so this change can be reverted for now.
8,9: (Php.stg) you are right, I not fully corrected those parts, and they are
never
used i my case
Original comment by rol...@gmail.com
on 14 Feb 2010 at 8:34
For items 1-3, strictly speaking these would be considered two distinct methods
in
Java because they have different signatures, i.e. one would not override the
other.
This has always been one of my biggest hurdles in converting Java code to PHP.
I don't know enough about the ANTLR runtime to say whether it matters, but in
this
situation it appears that maybe we should be calling the parent's method
whenever the
extra parameters are supplied.
For example, Lexer::matchAny() should not only optionally accept the $input
parameter, but also call parent::matchAny($input) when the parameter is
supplied, or
use its existing implementation otherwise.
Sidharth, let us know if you have any further feedback on this or any of the
other
items. To me it seems that I should make this described change for items 1-3,
pull
the github update for item #4, and undo items 5-9. Unless there are any
objections,
the result of that sounds like merge-to-trunk material.
Original comment by geoffspeicher
on 15 Feb 2010 at 10:08
I'm fine with the changes you described.
As a side note on matchAny, it blindly consumes the next token so there will
never be
any parameters. A better example would be match, which was renamed to
matchString and
matchChar. I think that when a method name is overloaded then the methods
should be
renamed, the parameter type works in many cases.
Original comment by sidharth...@gmail.com
on 16 Feb 2010 at 4:12
My mistake. Lexer::matchAny should be renamed to matchAnyChar. I'll file a
separate
ticket for that.
Original comment by sidharth...@gmail.com
on 16 Feb 2010 at 4:32
Rolland, now that you have commit access would you like to complete this or
should I
finish up the changes and merge to trunk?
Original comment by geoffspeicher
on 21 Feb 2010 at 1:00
merged branch with trunk
Original comment by rol...@gmail.com
on 25 Feb 2010 at 1:45
Original issue reported on code.google.com by
geoffspeicher
on 14 Feb 2010 at 1:19