areusch commented on issue #6653: URL: https://github.com/apache/incubator-tvm/issues/6653#issuecomment-706276834
My POV is that throwing in a destructor is bad because it will necessarily obscure any LOG(FATAL) raised during the lifetime of the object. This could happen from LOG(FATAL) in any invoked function as well. I think we should by convention not allow marking destructors as `DMLC_THROW_EXCEPTION` because of this behavior. It's implicit and hard to catch in a code review, and it means error reports take longer to root cause. The specific problem is that we are trying to set a string attribute value using a TVMObjectHandle. But, you can't see that because the destructor throws and we don't support chained exceptions. I see the benefits of writing the overall attrs mechanism this way, but it seems like the main use cases for using exceptions in the destructor are in invoking other visitors and in the AttrInitEntry object. In the AttrInitEntry object, we could just move the destructor logic to a Validate() function. In the AttrNonDefaultVisitor case, we could just build up a list of visitable fields in the AttrNonDefaultEntry destructor, and visit them a function defined on AttrNonDefaultVisitor. I don't believe visitors are recursive here, correct me if I am wrong. ---------------------------------------------------------------- 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]
