tqchen edited a comment on pull request #7084: URL: https://github.com/apache/tvm/pull/7084#issuecomment-747478194
Thanks @tkonolige @jroesch @ZihengJiang on discussing the issue of ICHECK and comments. While I in general agree that for cases that are user facing, having clear error message would be helpful. I would be hesitant to enforce error message for every internal invariance check. This decision is more of an engineering tradeoff. For example are many ways to improve code robustness and readability, code comment is one of them. However, it does not necessarily mean we need to comment every line of code, even though it is possible to do so and doing so might improve readability slightly. Developers may want to spend their attentions in other matters and we also need to balance these concerns, so as long as the code structure is clear, we could focus the code review on more important things -- e.g. documenting the interface clearly, making sure the overall logics are correct. Similarly, while it is always possible to add error message to every internal invariance checks, I don't think we are required to do so for every care. We can find many similar examples(like the ones below) in other large code base like TensorFlow and PyTorch, doing so doesn't make pytorch or tensorflow not product ready. - https://github.com/pytorch/pytorch/blob/master/torch/csrc/utils/throughput_benchmark.cpp#L25 - https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/common_runtime/replicate_per_replica_nodes_test.cc#L54 Of course this does not mean that we should not add error messages to the checks that could cause user facing errors. We should actually shift our attention to address these checks quickly, while giving flexibility to the developers in other cases that have minimum gain. In summary, **context of the code** matters, and I believe code review should focus on understanding the overall logics, trying to think about readability in a broader context(other than error message itself), while encouraging things to move on smoothly once things met a good quality. In this case we have discussed and I believe the ICHECK on pointer null invariant checking is not necessary. While it is indeed helpful to have better error message in other two cases. ---------------------------------------------------------------- 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
