See https://gitlab.com/antto/pimpmykicadbom/-/commit/c338027d737d02504fe92a6df00add2f59f462dd which has: From c338027d737d02504fe92a6df00add2f59f462dd Mon Sep 17 00:00:00 2001 From: cad <cad@whocares.gtfo> Date: Fri, 27 Sep 2024 18:16:33 +0300 Subject: [PATCH] throwing an exception might work better with a throw --- pimpmykicadbom.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pimpmykicadbom.cpp b/pimpmykicadbom.cpp index 04bed95..a904086 100644 --- a/pimpmykicadbom.cpp +++ b/pimpmykicadbom.cpp @@ -2593,7 +2593,7 @@ tbl_group_t BOM::prep(GROUP &g) // generate a hash/color if (pcfg == nullptr) { - std::logic_error("pcfg is nullptr"); + throw std::logic_error("pcfg is nullptr"); } const CFG &cfg = *pcfg; const vector<column_order_t> &v_cci = cfg.v_clrinfl; @@ -2975,7 +2975,7 @@ vector<column_order_t> gen_co(BOM &bom) vector<column_order_t> vec; if (bom.pcfg == nullptr) { - std::logic_error("pcfg is nullptr"); + throw std::logic_error("pcfg is nullptr"); } const CFG &cfg = *bom.pcfg; const vector<column_order_t> &v_co = cfg.v_colorder; It would be nice if we warned about such cases (if we don't already), perhaps something like: warning: possible missing `throw' [-Wsomething-or-other] std::logic_error("pcfg is nullptr"); ^^^^^^^^^^^^^^^^ throw From discussion on IRC: <dmalcolm> feels like a potential new warning for g++ if we don't have one already <jwakely> I don't think they do, because it has a non-trivial dtor. We could add [[nodiscard]] to the class declaration, but I think that doesn't actually work with gcc right now (it doesn't warn when it should) <dmalcolm> jwakely: is it an instance of a subclass derived from an "exception" base? <dmalcolm> or whatnot <jwakely> yes <jwakely> the non-trivial dtor makes the compiler think that creating a temporary that immediately goes out of scope might be done for its side effects <dmalcolm> (my knowledge of C++ exceptions is hazy, sorry) <jwakely> but we could either add [[nodiscard]] to it, or add a heuristic in the compiler that warns about any temporary of a type derived from std::exception <dmalcolm> as a heuristic: if the C++ FE sees a exception_subclass (args); by itself, warn did you mean "throw <that-thing>" ? <jwakely> since such types do not have useful side effects, and it's probably a typo like this case <jwakely> yup <jwakely> for belt and braces, you could check if the statement that creates that temporary exception object is the last statement of a block <jwakely> because if there are more statements in that block, maybe the author expected them to be reached. If they were intending to throw, they wouldn't have written unreachable statements after the throw <jwakely> probably overkill though <jwakely> in case I'm not being clear: if (some_condition) { std::range_error("blah"); func("this would be unreachable if the previous statement was a throw"); } <jwakely> but I can't think of a time when anybody would ever do that, so I think your original heuristic is good enough. A discarded temporary of a type that inherits from std::exception is probably a mistake
>We could add [[nodiscard]] to the class declaration, but I think that doesn't actually work with gcc right now (it doesn't warn when it should) That would be PR 85973 .
A simple example: #include <stdexcept> int divide(int num, int denom) { if (denom == 0) std::invalid_argument ("division by zero"); // forgot "throw" return num / denom; } which compiles with -Wall with gcc 14.2 without warnings: https://godbolt.org/z/c6r5PM97W constructing and desctructing the exception object, and then falling through to the division.
If we add [[nodiscard]] to the constructors of the standard exception types then we get a warning with no compiler changes needed: struct Exception { [[nodiscard]] Exception(const char*) { } }; int divide(int num, int denom) { if (denom == 0) { Exception("division by zero"); } return num / denom; } It should work to do `struct [[nodiscard]] Exception` instead of putting it on each constructor, but that's PR 85973
(In reply to Jonathan Wakely from comment #3) > If we add [[nodiscard]] to the constructors of the standard exception types > then we get a warning with no compiler changes needed: ...in Compiler Explorer as: https://godbolt.org/z/Po4oMPcox
(In reply to Jonathan Wakely from comment #3) > If we add [[nodiscard]] to the constructors of the standard exception types > then we get a warning with no compiler changes needed: Is it possible to do this to the standard library? Even if it is, maybe a warning for discarded temporary of a type that inherits from std::exception is still useful. A possible wording: warning: discarding temporary exception object; did you mean to `throw' it? [-Wmissing-throw] std::invalid_argument ("division by zero"); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ <fix-it hint suggesting prepending "throw "> note: `std::invalid_argument' is derived from `std::exception' <show the decl of std::invalid_argument> Probably shouldn't issue this warning for e.g.: named local that you forgot to throw: #include <stdexcept> int divide(int num, int denom) { std::invalid_argument exc ("division by zero"); return num / denom; } since the unused-variable warning should detect it.
Bug #119197 is related: it asks for `std::expected` to be marked [[nodiscard]]. Microsoft STL has marked their `std::expected`; and in the same commit they marked all their exception types as well. That is, Microsoft has adopted the direction given in jwakely's comment #3. I think libstdc++ should follow suit (which would also fix bug #119197).