Code review comment for lp:~zorba-coders/zorba/new-jsoniq

Hi Nicolae,

Looks good, thanks ;-) It's getting closer and closer to the git EBNFs!

Here are some comments:

1. Parser.y, line 2259: is "|| (block != NULL && block->isEmpty())" really needed? Will block not always be null if a BlockExpr is {} (see StatementsAndOptionalExpr nonterminal)?

2. Why is the OBJECT token treated specially and not mentioned in GeneralizedAtomicType like array/item/structured-item? Note that jn:object is no longer a function, as it was removed (I thought Markos removed it from Zorba, too). Then object should also be moved back to invalid function names.

Suggested test: object() throws a parsing error.

3. VersionDecl should not allow xquery in the JSONiq parser. Now, it seems to be allowed. A JSONiq query version declaration must be "jsoniq" (if a file begins with "xquery" the XQuery parser will be used anyway, so there is no way to test).

4. Why is append not authorized as a function? I thought it would be together with insert/rename/replace/delete.

Suggested test: local definition for local:append + default function namespace and query append().

5. "From" instead of "For" is implemented, but where is "select" instead of "return"? (this might require discussion about getting rid of "from", actually. I am not sure if the from/select decision was not reverted at some point.)

Suggested test: from $i in 1 to 10 select $i

6. Can you give me details and examples about the two new conflicts?

7. How is [[]] array handled? Is it parsed as a predicate with an array constructor and handled in the translator?

8. TRUE/FALSE/NULL/FROM(/SELECT?) should appear in the FUNCTION_NAME rule in a JSONIQ_PARSER #ifdef so that they can be used as function names. Or is this handled somewhere else?

Suggested test: false(), true(), null() -- from() with a local definition (see append above).

I hope it makes sense? Thanks!