ParserExpressionsTest fails

[vzickner]

The parser never consumes the "(". Its always the current token. For example when parseArguments() is called.

[norman465]

A lot of tests still fail simply because the tests are incorrect: E.g.: !new array[][]; is not valid, since the first [] expect an expression and there is none this.a >= this[]; Same here. The ArrayExpression [] expects an expression inside []. this[x] + this.y[] !null.callme() * -null.callme * true * false * true[][][].callme(); null.callme() This is an expression, but there needs to be a semicolon behind it to be valid.

[norman465]

@valentinz That was fixed in the commit a few minutes ago.

[effenok]

is this also valid ?

new int[42][42][42];

as new int[42] and then two array accesses?

[effenok]

fixed the test, now it passes :'''(

[effenok]

is this line correct?

identifier.m1().m2(42).m3(42,24).identifier[24+44].identifier[x][y][zZ];

[norman465]

new int[42][42][42];: yes, correct

identifier.m1().m2(42).m3(42,24).identifier[24+44].identifier[x][y][zZ];: should be correct as well

[effenok]

return;

und

return false;

funktionnieren nicht

[effenok]

so, esonetec fixed all the issues in all tests that I have now. Do we need more tests?

[norman465]

I think we need to run all the supplied minijava programs now and move forward from there.

[effenok]

from other groups ?

[norman465]

Yes, since our programs all pass.

[effenok]

test correct4 from another group fails:

============================= + testdata/pw-tests/correct_4.java + =======================================
Error in line: 29. Unexpected token ']' at character: 6
    at compiler.parser.Parser.parseUnaryExpression(Parser.java:546)
    at compiler.parser.Parser.parseExpression(Parser.java:457)
    at compiler.parser.Parser.parseExpression(Parser.java:452)
    at compiler.parser.Parser.parseArrayAccess(Parser.java:649)
    at compiler.parser.Parser.parsePostfixOp(Parser.java:586)
    at compiler.parser.Parser.parsePostfixExpression(Parser.java:569)
    at compiler.parser.Parser.parseUnaryExpression(Parser.java:538)
    at compiler.parser.Parser.parseExpression(Parser.java:457)
    at compiler.parser.Parser.parseExpression(Parser.java:452)
    at compiler.parser.Parser.parseBlockStatement(Parser.java:309)
    at compiler.parser.Parser.parseBlock(Parser.java:259)
    at compiler.parser.Parser.parseClassMember(Parser.java:108)
    at compiler.parser.Parser.parseProgram(Parser.java:53)
    at compiler.parser.Parser.parse(Parser.java:24)

[norman465]

It fails on B[] r = new B[i]; because our parser expects an IDENT after B. The fix will lead to SLL(3) if we find no way to adjust the grammar.

[effenok]

after which B? anyway, it is correct grammar => we have incorrect parser

[norman465]

The first B. Our parser currently can't distiguish between B[] and B[Expression] withour another token look ahead.

[norman465]

I fixed this issue. All programs pass now. Although this currently uses another token look ahead. We should try to alter the grammar so that we only need 1 token look ahead.

[andreas-eberle]

I would also propose to use a test coverage tool (for Eclipse "EclEmma" is great) to see which parts of the code still need to be tested. The parser currently only has 85% test coverage. This means there are several important paths not tested yet.

[vzickner]

I added some crazy code in the branch tests-sllfix. This code does not use the getLookAhead method from the lexer at all. Strategy: Consume the tokens and do, dependent on the token already read, the remaining part of work. What do you think about this?

[effenok]

can somebody post coverage report ? i can't make any cov. tool running

[esonetec]

@tests-sllfix: What is the main reason for changing our grammar from SSL(2) to SSL(1)? I thing it should be performance. Currently i can't image that tests-sllfix will produce faster results. Please look at parseBlockStatement: Now it is far more complicated and calls far more methods than with lookAhead in previous version. I would really like to perform a performance test, before changing the grammar to SSL(1). I created a ticket and am currently waiting for this performance test to be written, in order to measure the performance. I will volunteer to measure it, but someone else wanted to write this test.

[effenok]

didn't andreas wrote it?

[andreas-eberle]

I did only write a performance test for the lexer, yet. A simple test for the parser will follow this afternoon.

[vzickner]

@esonetec: The parseBlockStatement makes more calls, but this calls also done in your code before in sub-(sub-)*methods. So only the more sub methods in the other methods will slow done the code. The main problem is, that the precedence climbing is not SLL(1) and so, also with the new code, that is not SLL(1). We only removed the getLookAhead() method call.

[effenok]

stop being obsessed with performance! really !

[esonetec]

Beside performance i don't see you points to change our grammar to SSL(1).

@valentinz: Ok valid point. I don't have any problems to take your code. I just can't imagine if it makes parser faster. So why discussing, let us simply take some measurements? I thing there is no point to take fewer readable code, if the code doesn't cause performance changes.

[norman465]

@valentinz: I like the new code where we have more methods and no flags in methods calls. Could you add the resulting grammar to the wiki, so we can see how it looks before we decide which version we take?

[effenok]

ok, I just pushed simple performance tester in 6fa2bef12bcbbe0223bd4e57db78344c4ea6f133

Results of running file CorrectGrammar1.java appended to itself 1000 times:

results for testdata/parser5/CorrectGrammar1.java(repeat 1000 times) mean(ms) = 62.560 stdev(ms) = 21.191 stdev(%) = 33.87

unless someone finds a big error in my code, good luck finding out what is faster :)

(I usually calculate trimmed mean and trimmed stdev but currently I am too lazy to read the docs)

Edit: just to clarify -- the parser received 1000*the file and the parser was executed 100 times. The stats are from these 100 executions.

[esonetec]

I create two branches: tests_sllfix_performance (valentinz parser version) and tests_performance (current version). I couldn't measure any difference. (I know the performance tests have big stdev, but how else should we decide this question). On my machine i get means about 200ms for both versions. So i wouldn't change anything, as the change only makes the code less readable. Let's stick as close as it's possible to the original grammar.

(please don't merge these two branches to main and feel free to test it yourself)

[effenok]

Während ein Prüfung für Sprachtechnologie und Compiler, würdet ihr versuchen k zu reduzieren? Wäre der Antwort dann mit mehr "Punkten" bewertet?

[vzickner]

@norman465: The problem is, that you cannot represent the precedence climbing hack from my code in SLL(1). The main problem is, you have already an consumed token and do not know, which production in the precedence climbing it is. So i think its perhaps nicer to use the SLL(3) version. I can wrote an grammar for the things I wrote, but not before Saturday evening. In the code there is also an problem with implementing the AST. You have to use the already parsed left side of an token as parameter for the precedence climbing method. You have no other way to save this. This is maybe a little bit dirty. Resolution: I think its possible to wrote the code, so that you do not need an look ahead, but its not very useful. On the other side, what on a "large" k slows the code down?

Note: My changes from commit 2c02f6b31bf95abd635c2c8be6156af782704ba3 are easy to cherry pick in master without large modifications and eliminates one look ahead.