jroesch commented on pull request #6860: URL: https://github.com/apache/incubator-tvm/pull/6860#issuecomment-723230910
@giuseros The entire Relay AST has had spans for the entire existence of the IR, this change is a follow-on from UnifiedIR refactors where we make things more consistent. The span design originates (or Location) style of diagnostics is the style adopted by many modern compilers including Rust, and MLIR. The reason to have spans directly on the AST is the same reason to have type information they are important fields and having them be "intrinsic" vs. "extrinsic" properties. In my exp. working on compilers having things exist in global stateful maps which must be kept in sync introduces complexity as the global state must be passed everywhere and you have to be very careful at which time you read from the maps. For example propagating span information inside of a pass which builds new AST fragments is easy as you can directly build span information from existing spans. If we want to attach more meta-data for diagnostics I think we should attach that information to the diagnostic objects instead of attaching them to the spans/ast nodes directly. The diagnostics correspond to a location where some information was generated and the spans are indexes into the source representation. ---------------------------------------------------------------- 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: us...@infra.apache.org
