ChuanqiXu added a comment. In D108905#4655907 <https://reviews.llvm.org/D108905#4655907>, @MaskRay wrote:
> In D108905#4655694 <https://reviews.llvm.org/D108905#4655694>, @ChuanqiXu > wrote: > >> Oh, I am not saying the legacy and old comment. I mean you need to touch >> ReleaseNotes.rst and UserManual.rst since we add a new flag. Also we need >> either add a TODO/FIXME saying we need to emit an error in Sema if we find >> the the dtor of the exception we're throwing may throw or implement the >> semantics actually. > > Thanks for the reminder about `ReleaseNotes.rst` and `UsersManual.rst`! > > I think many changes don't update `UsersManual.rst` but this option is > probably quite useful and therefore deserves an entry. Added. > The primary change is to `clang/lib/CodeGen/ItaniumCXXABI.cpp`, which does > not report warnings. > If we want to implement a warning, we should probably do it in > clang/lib/Sema/SemaDeclCXX.cpp `Sema::BuildExceptionDeclaration`, which is > not touched in this file, so a TODO seems not appropriate... > > Is the warning to warn about `noexcept(false)` destructor in an > exception-declaration <https://eel.is/c++draft/except.pre>? When? > If at catch handlers, unfortunately we are cannot determine the exception > object for a `catch (...) { ... }` (used by coroutines). > Technically, even if a destructor is `noexcept(false)`, the destructor may > not throw when `__cxa_end_catch` destroys the object. > > So we probably should warn about throw expressions, which can be done in > `Sema::CheckCXXThrowOperand` and I will investigate it. > However, this warning appears orthogonal to `-fassume-nothrow-exception-dtor`. > We can argue that even without `-fassume-nothrow-exception-dtor`, an thrown > object with a `noexcept(false)` destructor is probably not a good idea. > However, I am on the fence whether the warning should be enabled-by-default. The idea of diagnosing such cases is originally raised by @rjmccall in https://github.com/llvm/llvm-project/issues/57375#issuecomment-1230590204. And in fact, John is suggesting an error instead of a warning. To be honest, in our downstream implementation, we didn't implement that semantical check neither. For the implementation, I think `Sema::CheckCXXThrowOperand` may be a good place and we can left a TODO there. Technically, we're using a dialect after we enable `-fassume-nothrow-exception-dtor`. In our dialect, we don't want to see an exception to throw exceptions. This is a rule for our dialect. And generally we have 2 strategies for implementing such language rules : - Detect it and emit errors after we found users violating the rules. - Don't detect it and claims it is the fault of the users. No matter what the strategy we choose, it is not orthogonal to `-fassume-nothrow-exception-dtor` for sure. It is highly related. Then, I think the first strategy is always a better choice. Generally we only choose 2 when we can't diagnose such cases. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108905/new/ https://reviews.llvm.org/D108905 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
