manupa-arm commented on pull request #9331: URL: https://github.com/apache/tvm/pull/9331#issuecomment-966187544
> I would question why we're adding them if they're in unreachable code paths, but if we do think they're worth adding I would consider them part of the contract of the internal function and thus should have associated tests for the behaviour (throwing an exception is now part of the functions behaviour). There is a fine line between defining a failing internal assert as a "behaviour" of the function and removing the assert just because it not executed in the present organization of the components. > If we're happy that at some point someone may inadvertently change the behaviour of this function, then potentially it's not a necessary line of code and we can apply the YAGNI principle from extreme programming Enforcing YAGNI to remove internal asserts will discourage developers from using asserts just because the execution path is not enforced today. In a summary I would support having internal assertions that makes it easier if another developer changes this code or re-uses some of it somewhere else. **If it means we need a add a borderline meaningful test that uses as an assertion to see if the assertion inside the code is triggered, lets do that then**. I guess what you are saying is we need a test for the above ICHECK in this PR? -- 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]
