ekalda commented on PR #12550: URL: https://github.com/apache/tvm/pull/12550#issuecomment-1243695451
Apologies, I was away last week, so just caught up with the discussion here. @mbaret I'm a bit confused by the addition of `CreateStorage` - you added handling (and testing) of nested functions, but we wouldn't reach that bit of code from `AOTLowerMain` since it doesn't support nested functions? It's not entirely clear to me where this came from. My opinion regarding to the testing has not changed - I agree with @mbaret in general that we shouldn't test implementation details, but I'd test `ExprAllocator` in this case since besides being an implementation detail it is a chunky pass with complex logic. However, I don't perceive it as a fundamental issue, so I have no intention to block this PR. I also don't think the "ensure test coverage of the new feature" of the code quality guidelines is particularly helpful here since it is not defined what "test coverage" means. So I'd approve this PR once I'll understand better what is going on with the `CreateStorage` (since clearly I'm missing something). -- 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]
