t-vi commented on pull request #6449: URL: https://github.com/apache/incubator-tvm/pull/6449#issuecomment-692548751
@zhiics @kevinthesun @masahi Thank you, @kevinthesun for your summary and all the work in the investigation and your PR. I think using `if isinstance(..., _expr.Expr):` would be very much preferable to using exceptions. 1. I see the uses of `try: ... except: ...` as using exceptions for regular control flow (because the error case is the one where the old normal logic is applicable and so we should have a clear view when the new logic is applicable and when not) and it would then except on the old cases, 2. Not using `try: ... except: ...` for regular control flow seems like good programming fundamentals to me. It would seem odd if TVM as a compiler stack would not strive to follow best practice here. Neither am I entirely sure whether 1. is contentious or not and to me it would seem that a PR is an odd place to form an opinion on 2. At the same time I see the construct as problematic enough to have a really hard time liking the current state of the PR. It would bring great joy if you could be convinced to move it to `if`. I should emphasize that I'm entirely for having the new functions appreciate your @kevinthesun work on this. Thank you! ---------------------------------------------------------------- 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: [email protected]
