KhronosGroup / NNEF-Docs

NNEF public repository
Apache License 2.0
14 stars 3 forks source link

tuple-rvalue-expression needs parens in argument-list #3

Closed jnorwood closed 6 years ago

jnorwood commented 6 years ago

The spec grammar shows tuple-rvalue-expression with optional parens. When an rvalue-expr is used in a comma separated argument-list this will be an error for the parsed results, which will see another tuple member instead of a new argument.

The tuple-rvalue-expression requires parens in this case.

In general, it would be more consistent/simple for the parser if tuples always required parens.

gyenesvi commented 6 years ago

I'd say if this is the only rule that causes left-recursion then it is worth considering fixing it, but I don;t think that's the case.

I don't understand why it can cause an actual problem. An expression like

some_operation(param1 = a, b, param2 = c)

will be invalid (parse error), because expression b will be interpreted as the next parameter, but that's not a problem, it just forces the use of parentheses in that context, like

some_operation(param1 = (a, b), param2 = c)

However, in other contexts it can be valid, such as

a, b = 1, 2

True that it could be written as

(a,b) = (1,2)

which may be more readable.

Do you know a case when it actually causes some problem which prohibits writing something valid, or where a parser would accept something invalid?

jnorwood commented 6 years ago

the input to the parser is being generated. If the grammar says that the parens are optional, then it seems to me that it is implying that they are just a matter of preference. However, in this case that you have already mentioned, the parens are not optional. It seems to me that it would be best to require parens on the generated input so that this case is avoided.

some_operation(param1 = (a, b), param2 = c)

gyenesvi commented 6 years ago

Yes, in case of generated input, the generator has to make sure that it is valid. True that it may be misleading to imply that it's fully optional.

gyenesvi commented 6 years ago

This has been made mandatory in the final version of the spec.

jnorwood commented 6 years ago

Yes, thanks. That will help.

jnorwood commented 6 years ago

verified fixed in new spec