[GitHub] [incubator-tvm] weberlo commented on pull request #5932: [Frontend][Relay] Add Parser 2.0

[GitBox <gi...@apache.org>]

weberlo commented on pull request #5932:
URL: https://github.com/apache/incubator-tvm/pull/5932#issuecomment-650369009


   A few thoughts:
   It's not clear to me that modifying this parser is any easier than the current parser.  One could make a case that the current parser is suboptimal, because ANTLR does a sort of "covering parse", and [_parser.py](https://github.com/apache/incubator-tvm/blob/master/python/tvm/relay/_parser.py) then does **another** stage of parsing that incorporates context, but I would argue there's value in this separation of concerns, because you no longer need to worry about the syntactic components of parsing (e.g., [precedence and associativity](https://github.com/apache/incubator-tvm/pull/5932/files#diff-807cc0a7f01f9113c1903d4614a3649dR749-R769)).
   
   Another benefit of using a parser generator like ANTLR is that you have a [specification](https://github.com/apache/incubator-tvm/blob/master/python/tvm/relay/grammar/Relay.g4) of the language that serves as documentation **and** defines the parsing behavior, keeping the documentation always up to date.
   
   I see the value in error reporting integration and removing the external dependency, but it would be good to further motivate these changes and maybe find ways to further modularize version 2.0 to make it noob-friendly.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org