chunit-quic commented on a change in pull request #9723:
URL: https://github.com/apache/tvm/pull/9723#discussion_r769709917
##########
File path: src/printer/relay_text_printer.cc
##########
@@ -389,12 +389,21 @@ Doc RelayTextPrinter::VisitExpr_(const TupleNode* op) {
if (op->fields.size() == 1) {
doc << ",";
}
- return doc << ")";
+ doc << ")";
+ if (op->span.defined()) {
Review comment:
Hi @mbs-octoml
Thank you for reviewing and giving this PR a positive feedback! :D
About comments in this part, please kindly correct me if I am wrong.
> nit:: can you leave a warning comment that we'll probably need to protect
this by some kind of 'include_spans' or 'verbose' printer flag
If I didn't misunderstand it. It will be nice to have a flag to control span
printing (After all some time it will be super long.)
In the latest commit I add a bool flag with true as default value to control
it. (src/printer/text_printer.h:116)
Although a "/* */" is left based on this implementation... is It basically
what we want?
> would you be up for doing the span suffix printing in the VisitExpr
override?
About this part, do you mean how about adding print span for those printer
without it currently?
Like, ScalarLiteral, PrintExpr and VisitExpr_(const IfNode* op) ...
If so, I did try to browse them at first. Yet it seems that it is not easy
to track and verify them comprehensively barely by a glance. Since sometime we
might even need to check their c++ global register and python end.
If is fine to you perhaps one more PR for this enhancement would be better?
I could also try to think about which test cases could help me to check those
kind of printers. :D
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
