On Sun, May 7, 2017 at 8:05 PM, Martin Sebor <mse...@gmail.com> wrote: > On 05/07/2017 02:03 PM, Volker Reichelt wrote: >> >> On 2 May, Martin Sebor wrote: >>> >>> On 05/01/2017 02:38 AM, Volker Reichelt wrote: >>>> >>>> Hi, >>>> >>>> catching exceptions by value is a bad thing, as it may cause slicing, >>>> i.e. >>>> a) a superfluous copy >>>> b) which is only partial. >>>> See also >>>> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#e15-catch-exceptions-from-a-hierarchy-by-reference >>>> >>>> To warn the user about catch handlers of non-reference type, >>>> the following patch adds a new C++/ObjC++ warning option >>>> "-Wcatch-value". >>> >>> >>> I think the problems related to catching exceptions by value >>> apply to (a subset of) class types but not so much to fundamental >>> types. I would expect indiscriminately warning on every type to >>> be overly restrictive. >>> >>> The Enforcement section of the C++ guideline suggests to >>> >>> Flag by-value exceptions if their types are part of a hierarchy >>> (could require whole-program analysis to be perfect). >>> >>> The corresponding CERT C++ Coding Standard guideline offers >>> a similar suggestion here: >>> >>> https://www.securecoding.cert.org/confluence/x/TAD5CQ >>> >>> so I would suggest to model the warning on that approach (within >>> limits of a single translation unit, of course). I.e., warn only >>> for catching by value objects of non-trivial types, or perhaps even >>> only polymorphic types? >>> >>> Martin >> >> >> I've never seen anybody throw integers in real-world code, so I didn't >> want to complicate things for this case. But maybe I should only warn >> about class-types. >> >> IMHO it makes sense to warn about non-polymorphic class types >> (although slicing is not a problem there), because you still have to >> deal with redundant copies. >> >> Another thing would be pointers. I've never seen pointers in catch >> handlers (except some 'catch (const char*)' which I would consider >> bad practice). Therefore I'd like to warn about 'catch (const A*)' >> which might be a typo that should read 'catch (const A&)' instead. >> >> Would that be OK? > > > To my knowledge, catch by value of non-polymorphic types (and > certainly fundamental types) is not a cause of common bugs. > It's part of the recommended practice to throw by value, catch > by reference, which is grounded in avoiding the slicing problem. > It's also sometimes recommended for non-trivial class types to > avoid creating a copy of the object (which, for non-trivial types, > may need to allocate resource and could throw). Otherwise, it's > not dissimilar to pass-by value vs pass-by-reference (or even > pass-by-pointer). Both may be good practices for some types or > in some situations but neither is necessary to avoid bugs or > universally applicable to achieve superior performance. > > The pointer case is interesting. In C++ Coding Standards, > Sutter and Alexandrescu recommend to throw (and catch) smart > pointers over plain pointers because it obviates having to deal > with memory management issues. That's sound advice but it seems > more like a design guideline than a coding rule aimed at directly > preventing bugs. I also think that the memory management bugs > that it might find might be more easily detected at the throw > site instead. E.g., warning on the throw expression below: > > { > Exception e; > throw &e; > } > > or perhaps even on > > { > throw *new Exception (); > } > > A more sophisticated (and less restrictive) checker could detect > and warn on "throw <T*>" if it found a catch (T) or catch (T&) > in the same file and no catch (T*) (but not warn otherwise). > > Martin > > PS After re-reading some of the coding guidelines on this topic > it occurs to me that (if your patch doesn't handle this case yet) > it might be worth considering to enhance it to also warn on > rethrowing caught polymorphic objects (i.e., warn on > > catch (E &e) { throw e; } > > and suggest to use "throw;" instead, for the same reason: to > help avoid slicing. > > PPS It may be a useful feature to implement some of other ideas > you mentioned (e.g., throw by value rather than pointer) but it > feels like a separate and more ambitious project than detecting > the relatively common and narrow slicing problem.
I agree with Martin's comments. Jason
