public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: "Jason Merrill via gcc-patches" <gcc-patches@gcc.gnu.org>
To: Martin Sebor <msebor@gmail.com>
Cc: Volker Reichelt <v.reichelt@netcologne.de>,
	gcc-patches List <gcc-patches@gcc.gnu.org>
Subject: Re: {PATCH] New C++ warning -Wcatch-value
Date: Mon, 08 May 2017 13:14:00 -0000	[thread overview]
Message-ID: <CADzB+2=YUigdtp3=E3N6vR_CfSLVscqLMEgkmXjz4rDwvEQ9ng@mail.gmail.com> (raw)
In-Reply-To: <c5a2c640-1ac3-ec0b-0215-e2c2d270b25d@gmail.com>

On Sun, May 7, 2017 at 8:05 PM, Martin Sebor <msebor@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

  reply	other threads:[~2017-05-08 13:14 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-01  8:38 Volker Reichelt
2017-05-03  3:34 ` Martin Sebor
2017-05-07 20:28   ` Volker Reichelt
2017-05-08  3:18     ` Martin Sebor
2017-05-08 13:14       ` Jason Merrill via gcc-patches [this message]
2017-05-14 15:30       ` Volker Reichelt
2017-05-15 20:03         ` Martin Sebor
2017-05-24 20:13           ` Jason Merrill
2017-05-30  6:22             ` Volker Reichelt
2017-05-31 20:55               ` Jason Merrill
2017-06-05 10:55           ` Volker Reichelt
2017-06-05 19:08             ` Jason Merrill

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CADzB+2=YUigdtp3=E3N6vR_CfSLVscqLMEgkmXjz4rDwvEQ9ng@mail.gmail.com' \
    --to=gcc-patches@gcc.gnu.org \
    --cc=jason@redhat.com \
    --cc=msebor@gmail.com \
    --cc=v.reichelt@netcologne.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).