Warning on C++ catch by value on non primitive types

Warning on C++ catch by value on non primitive types

[Thomas Costa]

I posted this on the gcc-help list but I haven't received any replies  
so I thought that might mean that I hadn't missed anything obvious  
and gcc doesn't have such a warning at present.
Is this correct?
What are the chances of adding such a warning?
Is it basic enough to add to the 3.4.x and/or 4.x C++ front end that  
I could help with the task?


> From: Thomas Costa <tcosta@ovro.caltech.edu>
> Date: 10 October 2005 3:34:46 PM PDT
> To: gcc-help@gcc.gnu.org
> Subject: Warning on C++ catch by value on non primitive types
>
>
> Is there any way to get gcc/g++ to warn on C++ code that does a  
> catch by value of non primitive types (i.e. types that aren't int,  
> float, bool, etc.)?
>
> I would take a warning on any catch by value but the ideal would be  
> a warning that doesn't fire for "catch (const int x)"
>
> This continually creeps into our our C++ code base.
>
>
>

Re: Warning on C++ catch by value on non primitive types

[Mike Stump]

On Oct 12, 2005, at 2:20 PM, Thomas Costa wrote:
> gcc doesn't have such a warning at present.

Yup.

> Is this correct?


I suspect so.

> What are the chances of adding such a warning?

Zero, unless someone else wants it or you file a bug report asking  
for the enhancement.  Be sure to explain why it is a good idea,  
otherwise the chances of it happening could drop to zero.

> Is it basic enough to add to the 3.4.x and/or 4.x C++ front end  
> that I could help with the task?

Sure.

Look at the parser (gcc/cp/parser.c:cp_parser_handler) and set a  
break point there, follow it down into semantic processing, pick a  
nice place to do the checking, and then add the code to check it.

For it to be accepted, others would have to agree it is a good thing  
to have, for example, if it were in the Effective C++ book, you can  
argue that way.

Re: Warning on C++ catch by value on non primitive types

[Nathan Sidwell]

Mike Stump wrote:

> Look at the parser (gcc/cp/parser.c:cp_parser_handler) and set a  break 
> point there, follow it down into semantic processing, pick a  nice place 
> to do the checking, and then add the code to check it.

look at finish_handler_parms in semantics.c

> For it to be accepted, others would have to agree it is a good thing  to 
> have, for example, if it were in the Effective C++ book, you can  argue 
> that way.

yeah, if it were in one of those books it could be added to the -weff-c++ 
option.  It doesn't seem sensible to add a different option for an alternative 
(set of?) coding rule(s).

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: Warning on C++ catch by value on non primitive types

[Avi Kivity]

Kai Henningsen wrote:

>So what you say is that any decent modern C++ coding guide/list wants to  
>forbid catching the C++ standard exceptions, and anything derived from  
>them?
>
>  
>
no, only catch by value is problematic, as it can lead to slicing.

catch by reference is perfectly fine.

Re: Warning on C++ catch by value on non primitive types

[Kai Henningsen]

tcosta@ovro.caltech.edu (Thomas Costa)  wrote on 13.10.05 in <01A7E769-CA3B-4994-955B-69387D2B7E07@ovro.caltech.edu>:

> On 13 Oct 2005, at 7:41 AM, Benjamin Kosnik wrote:
>
> >
> >
> >> yeah, if it were in one of those books it could be added to the -
> >> weff-c+
> >> + option. It doesn't seem sensible to add a different option for an
> >> alternative (set of?) coding rule(s).
> >>
> >
> > FYI this is item 13 in MEC++.
> >
>
> It is on just about any decent modern C++ coding guide/list somewhere.
>
> > I think this would be a good error to have. My suggestion is to
> > file an
> > enhancement request in gcc bugzilla, with this code:
> >
> > #include <stdexcept>
> >
> > void
> > foo()
> > {
> >   try
> >     {
> >     }
> >   catch (std::logic_error e)
> >     {
> >     }
> > }

So what you say is that any decent modern C++ coding guide/list wants to  
forbid catching the C++ standard exceptions, and anything derived from  
them?

How on earth can that count as "decent"?!

MfG Kai

Re: Warning on C++ catch by value on non primitive types

[Joe Buck]

On Fri, Oct 14, 2005 at 01:55:49AM +0200, Gabriel Dos Reis wrote:
> What I find the most annoying with -Weffc++ is not the rules
> themselves; but the fact that is that it is a set of guidelines
> designed for a *particular style* of programming in C++ 

That is, traditional OO programming.

> many people see it as _The Right Way_ of doing things in C++, forgetting
> this is a multi-paradigm language

In particular, generic programming, as represented in STL and Boost.

> I do not find it surprising
> that major software components like libstdc++ or Boost do not
> slavishly follow -Weffc++ -- I would have been surprised if they did.

It seems that the key issue with -Weffc++ is that it is overly paranoid
about the possibility that someone will destroy a derived object through
a base pointer or reference, resulting in the wrong destructor being
called, even in cases where neither the base class nor the derived class
have any virtual functions.  In the case of no virtual functions, the
base/derived relationship is just a means of factoring out common code;
a typical STL example would be the deriving of a function object from
something like std::binary_function<T1,T2,Tresult>, or in the construction
of reverse iterators.

Furthermore, there are many cases where calling the wrong destructor is
a no-op in any real GCC implementation (e.g. both the base and derived
classes have one data member, a POD, and there is no explicit destructor,
or the only side effect is in the base destructor).  Most of the
STL-related noises -Weffc++ makes were, as I recall, related to such
cases.  Maybe it could be made to shut up only about particular cases
known to be safe.

Re: Warning on C++ catch by value on non primitive types

[Gabriel Dos Reis]

Benjamin Kosnik <bkoz@redhat.com> writes:

| As you are well aware, GCC is part of the GNU project. It's all about
| freedom.

I understand that.  But that should not prevent us from getting
perspective on -Weffc++.  

-- Gaby

Re: Warning on C++ catch by value on non primitive types

[Gabriel Dos Reis]

Joe Buck <Joe.Buck@synopsys.COM> writes:

| So please don't build anything new on top of this ill-considered warning.

Amen.

-- Gaby

Re: Warning on C++ catch by value on non primitive types

[Gabriel Dos Reis]

Mike Stump <mrs@apple.com> writes:

| On Oct 13, 2005, at 8:58 AM, Joe Buck wrote:
| > -Weffc++ is broken and should just die.
| 
| Or we can `modernize it' by removing those things we no longer agree
| with, and document the bits of it that we don't like anymore in the
| manual.

I find that to be invitation to confusion.  People who the rules or
have been trained to learn the rules would be needlessly confused.
And people who don't know them would be confused too.  Call it
something else you shall.

-- Gaby

Re: Warning on C++ catch by value on non primitive types

[Gabriel Dos Reis]

Thomas Costa <tcosta@ovro.caltech.edu> writes:

| That would be great and I will ask very, very nicely and offer to
| help any way I can however...
| I didn't want this totally tied to -Weffc++ unless there was also a
| separate switch for turning it on/off because:
| 
| 1.) Last time I checked g++'s own standard library headers did not
| pass -Weffc++ cleanly and hence I couldn't get a clean build of my C+
| + code using -Weffc++.
| 2.) A lot of other 3rd party C++ headers including the last version
| of Boost I was using (admittedly a relatively old version, maybe
| version 1.30) didn't pass -Weffc++ cleanly.
| 3.) People have many times debated the validity/usefulness of some of
| the warnings that -Weffc++ produces and I think it's better to have
| individual switches for each warning in -Weffc++ and then -Weffc++
| just becomes a batch switch that turns on the individual switches en
| masse.

What I find the most annoying with -Weffc++ is not the rules
themselves; but the fact that is that it is a set of guidelines
designed for a *particular style* of programming in C++ -yet-
many people see it as _The Right Way_ of doing things in C++, forgetting
this is a multi-paradigm language and not a Pure <fill-in your
favorite buzzword> programming language.  I do not find it surprising
that major software components like libstdc++ or Boost do not
slavishly follow -Weffc++ -- I would have been surprised if they did.

-- Gaby

Re: Warning on C++ catch by value on non primitive types

[Benjamin Kosnik]

> See comment #4 to PR 12854.  You state, correctly, that "The remaining
> bits that warn with -Weffc++ are either mandated by the standard, or
> required for performance/size reasons by ABI implementations."

Yep.

> That is, some aspects of -Weffc++ aren't compatible with the C++ standard;
> others require that classes that should not have a virtual function table
> add one.

Yep.
 
> You state later in the log that the flag is now usable on mainline.  But
> if I understand correctly, this is because of the warning suppression in
> system headers, and is true only of code that doesn't use the STL heavily
> (in particular, deriving from function objects and the like).

Nyet.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14172

> I was never crazy about the idea of treating system headers differently
> for the purpose of warnings.  I understand why it is done and why it is
> necessary, but if the GCC project itself cannot produce warning-free
> code for system libraries, it suggests that there is something wrong with
> the warning.  So it's a tradeoff: we can't fix headers provided by others
> (OS headers), so it's right to shut up about them.  And certainly for
> specialized warnings it's understandable that the system header won't
> match.  But at least for warnings in -Wall, I think the system headers
> should pass, and some of the -Weffc++ warnings could legitimately go in
> -Wall.

Agreed. Doing the pragma system header thing was a sad day for me. 

For major releases, I try to run with highest warning flags on both GNU
and EDG compilers, and also bring out the tweaky flags like -Weffc++.
IMHO this kind of effort is only really worthwhile on rare occasions,
and is overkill for day-to-day development.
 
> > The alternatives, which include people writing their own tools or
> > purchasing proprietary C++ lint tools to do this for them (which still
> > have the problems of the current g++ warnings, if not more) are not
> > suggestions I consider wise.
> 
> I agree.  Something like -Weffc++ could be a very nice idea.  The problem
> is that the original Effective C++ rules have bugs.

Agreed. However, we can fix the implementation of some of these rules
to make them more sane. If we can, we should, and we should definitely
encourage people to work on this stuff instead of shutting them down
with "kill all warnings" talk.

> > Perhaps you were burned with older implementations of this flag. While
> > still not perfect, a lot of improvements went into this warning for
> > 3.4.x.
> 
> And thanks for doing them.

Thanks to Giovanni for doing the heavy lifting. All I did was complain
in the right place, with enough detail that other people could
understand my complaints.
 
> > The simple example below doesn't warn, for instance.
> > 
> > struct base
> > {
> > 	void foo();
> > };
> > 
> > struct derived: public base
> > {
> > 	void bar();
> > };
> 
> Right; the warnings come for certain uses of derived.  When I have time,
> I'll go back over the issues I've had and try to show some examples.

That would be nice. Make sure you put them in bugzilla with -Weffc++ in the keyword.

I seem to dimly recall that one of the remaining issues with bogus
warnings is with the current std::allocator design. That might be a
good first place to start with this examples.

It does seem like the highest-priority item for going forward is
splitting -Weffc++ into smaller, more granular units so that it is
possible for people to use just the useful warnings, and not get what
they feel is noise. As Paolo has pointed out, there is already a
feature request for this very thing: c+ +/16166.

best,
benjamin

Re: Warning on C++ catch by value on non primitive types

[Joe Buck]

I wrote:
> > -Weffc++ is broken and should just die. 

On Thu, Oct 13, 2005 at 02:31:07PM -0500, Benjamin Kosnik wrote:
> If you search bugzilla for "Weffc++" you can get an idea of what are
> considered useful improvements. I don't consider dated diatribes like
> your previous email especially helpful.

See comment #4 to PR 12854.  You state, correctly, that "The remaining
bits that warn with -Weffc++ are either mandated by the standard, or
required for performance/size reasons by ABI implementations."

That is, some aspects of -Weffc++ aren't compatible with the C++ standard;
others require that classes that should not have a virtual function table
add one.

You state later in the log that the flag is now usable on mainline.  But
if I understand correctly, this is because of the warning suppression in
system headers, and is true only of code that doesn't use the STL heavily
(in particular, deriving from function objects and the like).

I was never crazy about the idea of treating system headers differently
for the purpose of warnings.  I understand why it is done and why it is
necessary, but if the GCC project itself cannot produce warning-free
code for system libraries, it suggests that there is something wrong with
the warning.  So it's a tradeoff: we can't fix headers provided by others
(OS headers), so it's right to shut up about them.  And certainly for
specialized warnings it's understandable that the system header won't
match.  But at least for warnings in -Wall, I think the system headers
should pass, and some of the -Weffc++ warnings could legitimately go in
-Wall.

> The alternatives, which include people writing their own tools or
> purchasing proprietary C++ lint tools to do this for them (which still
> have the problems of the current g++ warnings, if not more) are not
> suggestions I consider wise.

I agree.  Something like -Weffc++ could be a very nice idea.  The problem
is that the original Effective C++ rules have bugs.

> Perhaps you were burned with older implementations of this flag. While
> still not perfect, a lot of improvements went into this warning for
> 3.4.x.

And thanks for doing them.

> The simple example below doesn't warn, for instance.
> 
> struct base
> {
> 	void foo();
> };
> 
> struct derived: public base
> {
> 	void bar();
> };

Right; the warnings come for certain uses of derived.  When I have time,
I'll go back over the issues I've had and try to show some examples.

Re: Warning on C++ catch by value on non primitive types

[Benjamin Kosnik]

> -Weffc++ is broken and should just die. 

-Weffc++ should be made more useful, and able to be switched on a per-rule basis.

If you search bugzilla for "Weffc++" you can get an idea of what are
considered useful improvements. I don't consider dated diatribes like
your previous email especially helpful.

As you are well aware, GCC is part of the GNU project. It's all about
freedom. We're not suggesting that people always use this flag, merely
that if they are motivated, know some C++, and are willing to look
through results that may contain false positives, they will no doubt
find some useful information. I know I do. I don't use this flag all
the time, but do use it on occasion and find the results interesting.

And you know what? This isn't the only C++ language flag that has these
issues. Look at -Wabi and the inlining warnings for similar issues.
There are no doubt other issues with other warnings: every long term
user of GCC no doubt has some favorite carp about the ridiculousness of
some flag or another. Ditching diagnostics because you don't like them
or they are imperfectly implemented seems like a strange course of
action for a member of the gcc SC to advocate.

The alternatives, which include people writing their own tools or
purchasing proprietary C++ lint tools to do this for them (which still
have the problems of the current g++ warnings, if not more) are not
suggestions I consider wise.

> The coding rules they propose,
> and that the flag enforces, are completely incompatible with the STL: you
> cannot write a C++ standard library in a way that doesn't either spew tons
> of -Weffc++ warnings or that bloats out most of the objects by adding an
> unnecessary virtual function table to all kinds of objects that are now a
> single pointer (you're pretty much forced to put a virtual destructor in
> any class that is inherited from).  That's two pointers instead of one
> in most iterators and function objects.

Perhaps you were burned with older implementations of this flag. While
still not perfect, a lot of improvements went into this warning for
3.4.x.

I thought this particular complaint got fixed in the last round of -
Weffc++ enhancements, where this only warns if derived classes have
virtual functions. I can't find a reference to it in the mailing list
or in bugzilla.

The simple example below doesn't warn, for instance.

struct base
{
	void foo();
};

struct derived: public base
{
	void bar();
};

-benjamin

Re: Warning on C++ catch by value on non primitive types

[Mike Stump]

On Oct 13, 2005, at 11:46 AM, Thomas Costa wrote:
> So can the existing set of warnings that -Weffc++ enables be broken  
> up into fine grained warnings quite easily?

Yes.  M-x grep warn_ecpp in gcc/cp/*, then replace:

         warning (0,

with

         warning (OPT_Weffc_12,

and then arrange for an -Weffc_12 flag (or whatever name you like).   
If his new book removes one you don't like, just whack it instead.   
We can default ones that go against sane C++ to off, that way, we can  
restore the utility of the -Weffc++ flag.

> Has it already been done?

No.

Sound good?

Re: Warning on C++ catch by value on non primitive types

[Paolo Carlini]

Mike Stump wrote:

> ... Hopefully he'll release a new book with updated guidelines.

The new, third, edition of Effective C++ is out.

Paolo.

Re: Warning on C++ catch by value on non primitive types

[Paolo Carlini]

Thomas Costa wrote:

> So can the existing set of warnings that -Weffc++ enables be broken 
> up into fine grained warnings quite easily? Has it already been done?

c++/16166

Paolo.

Re: Warning on C++ catch by value on non primitive types

[Mike Stump]

On Oct 13, 2005, at 8:58 AM, Joe Buck wrote:
> -Weffc++ is broken and should just die.

Or we can `modernize it' by removing those things we no longer agree  
with, and document the bits of it that we don't like anymore in the  
manual.  Hopefully he'll release a new book with updated guidelines.   
If there are no useful things left, well, let's remove it (or remove  
the effects the option has).

Re: Warning on C++ catch by value on non primitive types

[Thomas Costa]

So can the existing set of warnings that -Weffc++ enables be broken  
up into fine grained warnings quite easily?
Has it already been done?

On 13 Oct 2005, at 11:44 AM, Mike Stump wrote:

> On Oct 13, 2005, at 8:57 AM, Thomas Costa wrote:
>
>> I didn't want this totally tied to -Weffc++ unless there was also  
>> a separate switch for turning it on/off because:
>>
>
> We support fine grained warnings now.  So, this isn't a problem.   
> Just be sure to mention you want a fine grained flag, and that you  
> like spelling "-W..." (after you review the way we spell options).
>

Re: Warning on C++ catch by value on non primitive types

[Mike Stump]

On Oct 13, 2005, at 8:57 AM, Thomas Costa wrote:
> I didn't want this totally tied to -Weffc++ unless there was also a  
> separate switch for turning it on/off because:

We support fine grained warnings now.  So, this isn't a problem.   
Just be sure to mention you want a fine grained flag, and that you  
like spelling "-W..." (after you review the way we spell options).

Re: Warning on C++ catch by value on non primitive types

[Joe Buck]

On Thu, Oct 13, 2005 at 09:41:22AM -0500, Benjamin Kosnik wrote:
> 
> > yeah, if it were in one of those books it could be added to the -weff-c+
> > + option. It doesn't seem sensible to add a different option for an
> > alternative (set of?) coding rule(s).
> 
> FYI this is item 13 in MEC++.

-Weffc++ is broken and should just die.  The coding rules they propose,
and that the flag enforces, are completely incompatible with the STL: you
cannot write a C++ standard library in a way that doesn't either spew tons
of -Weffc++ warnings or that bloats out most of the objects by adding an
unnecessary virtual function table to all kinds of objects that are now a
single pointer (you're pretty much forced to put a virtual destructor in
any class that is inherited from).  That's two pointers instead of one
in most iterators and function objects.

Yes, you can tell the compiler to ignore warnings in system headers,
but then users can't use the same programming techniques that are used
in the STL.

So please don't build anything new on top of this ill-considered warning.

Re: Warning on C++ catch by value on non primitive types

[Thomas Costa]

On 13 Oct 2005, at 7:41 AM, Benjamin Kosnik wrote:

>
>
>> yeah, if it were in one of those books it could be added to the - 
>> weff-c+
>> + option. It doesn't seem sensible to add a different option for an
>> alternative (set of?) coding rule(s).
>>
>
> FYI this is item 13 in MEC++.
>

It is on just about any decent modern C++ coding guide/list somewhere.

> I think this would be a good error to have. My suggestion is to  
> file an
> enhancement request in gcc bugzilla, with this code:
>
> #include <stdexcept>
>
> void
> foo()
> {
>   try
>     {
>     }
>   catch (std::logic_error e)
>     {
>     }
> }
>
>
> saying that with -Weffc++, you want a warning. Include a link back to
> this thread, so that who-ever works on this can read the initial
> reaction and the suggestion by Nathan to hook into  
> finish_handler_parms.
>

Yes, thanks for all the feedback.
I will look at the code mentioned earlier.

> In the near past, Giovanni has done a good job of enhancing the More
> Effective C++ rules. If you ask nicely, maybe he'd do the same for  
> this.
>

That would be great and I will ask very, very nicely and offer to  
help any way I can however...
I didn't want this totally tied to -Weffc++ unless there was also a  
separate switch for turning it on/off because:

1.) Last time I checked g++'s own standard library headers did not  
pass -Weffc++ cleanly and hence I couldn't get a clean build of my C+ 
+ code using -Weffc++.
2.) A lot of other 3rd party C++ headers including the last version  
of Boost I was using (admittedly a relatively old version, maybe  
version 1.30) didn't pass -Weffc++ cleanly.
3.) People have many times debated the validity/usefulness of some of  
the warnings that -Weffc++ produces and I think it's better to have  
individual switches for each warning in -Weffc++ and then -Weffc++  
just becomes a batch switch that turns on the individual switches en  
masse.


> best,
> benjamin
>

Re: Warning on C++ catch by value on non primitive types

[Benjamin Kosnik]

> yeah, if it were in one of those books it could be added to the -weff-c+
> + option. It doesn't seem sensible to add a different option for an
> alternative (set of?) coding rule(s).

FYI this is item 13 in MEC++.

I think this would be a good error to have. My suggestion is to file an
enhancement request in gcc bugzilla, with this code:

#include <stdexcept>

void
foo()
{
  try
    {
    }
  catch (std::logic_error e)
    {
    }
}


saying that with -Weffc++, you want a warning. Include a link back to
this thread, so that who-ever works on this can read the initial
reaction and the suggestion by Nathan to hook into finish_handler_parms.

In the near past, Giovanni has done a good job of enhancing the More
Effective C++ rules. If you ask nicely, maybe he'd do the same for this.

best,
benjamin