Bug 16166 - -Weffc++ finer granularity
Summary: -Weffc++ finer granularity
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.0.0
: P2 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
: 63871 (view as bug list)
Depends on:
Blocks: Weffc++ 12854
  Show dependency treegraph
 
Reported: 2004-06-23 22:26 UTC by Benjamin Kosnik
Modified: 2022-05-09 08:18 UTC (History)
6 users (show)

See Also:
Host: i686-pc-linux-gnu
Target: i686-pc-linux-gnu
Build: i686-pc-linux-gnu
Known to work:
Known to fail:
Last reconfirmed: 2005-04-30 16:12:36


Attachments
Patch to split -Weffc++ into separate options. (2.10 KB, patch)
2018-02-16 13:36 UTC, Jonathan Wakely
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Kosnik 2004-06-23 22:26:04 UTC
It would be nice to be able to warn for just a particular EFC++ warning, not all
at once.

For instance, to warn for item 11:

-Weffc++11 

or 

-Weffc++=11

or whatever.

See:
http://gcc.gnu.org/ml/libstdc++/2004-04/msg00067.html
-benjamin
Comment 1 Benjamin Kosnik 2004-06-24 02:08:50 UTC
It looks like there are warnings for the following items:

# Item 11: Define a copy constructor and an assignment operator for classes with
dynamically allocated memory.
# Item 12: Prefer initialization to assignment in constructors.
# Item 14: Make destructors virtual in base classes.
# Item 15: Have operator= return a reference to *this.
# Item 23: Don't try to return a reference when you must return an object.
# Item 6: Distinguish between prefix and postfix forms of increment and
decrement operators.
# Item 7: Never overload &&, ||, or ,.

So, there would be (at least) seven options, plus all.

Comment 2 Kriang Lerdsuwanakij 2004-07-31 12:16:52 UTC
Confirmed.
Comment 3 Jonathan Wakely 2011-11-07 16:10:10 UTC
Reviewing these warnings w.r.t the much improved third edition...

(In reply to comment #1)
> # Item 11: Define a copy constructor and an assignment operator for classes with
> dynamically allocated memory.

Replaced by Item 14: "Think carefully about copying behavior in resource-managing classes" - the advice is less specific, but more useful.  I'm not sure how to turn it into a warning though!

> # Item 12: Prefer initialization to assignment in constructors.

Replaced by Item 4: "Make sure that objects are initialized before they're used", and G++ misinterprets the original item anyway and warns about *any* member without a mem-initializer, which is very annoying: there's no point initializing a std::string, it has a perfectly safe default constructor.  My -Wmeminit patch for PR 2972 should replace the current warning for this item, as it only warns about members left uninitialized by the constructor.

> # Item 14: Make destructors virtual in base classes.

Adjusted to Item 7: "Declare destructors virtual in polymorphic base classes", the warning is still relevant (but -Wdelete-non-virtual-dtor is more useful IMHO)

> # Item 15: Have operator= return a reference to *this.

Renumbered to Item 10. Still relevant.

> # Item 23: Don't try to return a reference when you must return an object.

Renumbered to Item 21. Still relevant.

> # Item 6: Distinguish between prefix and postfix forms of increment and
> decrement operators.
> # Item 7: Never overload &&, ||, or ,.

These are from More Effective C++ which only has one edition.
Comment 4 David Stone 2012-05-29 02:13:53 UTC
I would recommend against naming each warning -Weffc++[n], but rather, give a more descriptive name. My suggestion is to create a few warnings, so that -Weffc++ would map to the following set of warnings (with their current description for reference and my suggested name):

Effective C++:

* Item 11: Define a copy constructor and an assignment operator for classes with dynamically allocated memory.

-Wcopy-resource-class

* Item 12: Prefer initialization to assignment in constructors.

-Wassignment-in-constructor

* Item 14: Make destructors virtual in base classes.

Already covered by -Wnon-virtual-dtor

* Item 15: Have operator= return a reference to *this.

-Wassignment-operator-return-value

* Item 23: Don't try to return a reference when you must return an object. 

-Wsuspicious-reference-return

More Effective C++:

* Item 6: Distinguish between prefix and postfix forms of increment and decrement operators.

-Wprefix-postfix

* Item 7: Never overload &&, ||, or ,. 

Perhaps this should be split into two warnings of its own:
-Woverloaded-short-circuit
-Woverloaded-comma

In summary, you could simulate exactly the behavior of -Weffc++ by turning on each of these warnings individually, or you could turn on -Weffc++ and selectively turn off a few warnings that you don't want.
Comment 5 Jonathan Wakely 2012-05-29 08:34:32 UTC
(In reply to comment #4)
> * Item 11: Define a copy constructor and an assignment operator for classes
> with dynamically allocated memory.
> 
> -Wcopy-resource-class

IMHO this warning should just go. With deleted copy ctor/assign and move ctor/assign there are even more places where a hard and fast rule isn't useful.
 
> * Item 12: Prefer initialization to assignment in constructors.
> 
> -Wassignment-in-constructor

If I ever get my -Wmeminit patch working properly it would provide this.

> * Item 14: Make destructors virtual in base classes.
> 
> Already covered by -Wnon-virtual-dtor

And the more useful -Wdelete-non-virtual-dtor

> In summary, you could simulate exactly the behavior of -Weffc++ by turning on
> each of these warnings individually, or you could turn on -Weffc++ and
> selectively turn off a few warnings that you don't want.

Yep, that would be much better
Comment 6 Manuel López-Ibáñez 2012-05-29 09:58:08 UTC
(In reply to comment #4)
> I would recommend against naming each warning -Weffc++[n], but rather, give a
> more descriptive name. My suggestion is to create a few warnings, so that
> -Weffc++ would map to the following set of warnings (with their current
> description for reference and my suggested name):
> 

David, if you wish to implement such a patch (or, even better, series of patches, one for each new option), the only changes needed are:

* In the particular warning () calls, replace OPT_Weffc__ with the appropriate OPT_W option.

* In c-family/c.opt, add a new entry for the new Wfoo option and use EnabledBy(Weffc++)

* In doc/invoke.texi, document the new options.

* Bootstrap+regression test and submit to gcc-patches.

Profit!
Comment 7 David Stone 2012-05-29 20:57:22 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > * Item 11: Define a copy constructor and an assignment operator for classes
> > with dynamically allocated memory.
> > 
> > -Wcopy-resource-class
> 
> IMHO this warning should just go. With deleted copy ctor/assign and move
> ctor/assign there are even more places where a hard and fast rule isn't useful.
> 

And in general, classes should just use a smart pointer from boost / the standard library, rather than managing their own memory directly. I agree that this warning probably isn't that useful, and I wouldn't be sad to see it go.

> > * Item 12: Prefer initialization to assignment in constructors.
> > 
> > -Wassignment-in-constructor
> 
> If I ever get my -Wmeminit patch working properly it would provide this.

Is the issue just finding the time to do it, or are there subtle issues involved?

> > * Item 14: Make destructors virtual in base classes.
> > 
> > Already covered by -Wnon-virtual-dtor
> 
> And the more useful -Wdelete-non-virtual-dtor

Yeah, my only thought for the usefulness of -Wnon-virtual-dtor over -Wdelete-non-virtual-dtor is that a library author may wish to have a class that users are supposed to derive from, but doesn't actually call delete anywhere in the library code. They may want to ensure that users of the library (who may not have any warnings turned on at all) do not run into any surprising bugs.

(In reply to comment #6)
> David, if you wish to implement such a patch (or, even better, series of
> patches, one for each new option), the only changes needed are:
> 

I am currently writing up a patch to update doc/invoke.texi for warnings, to try and make it easier to see what is turned on by -Wall and -Wextra and what isn't. Do you think it would be best for me to submit a patch based on the current documentation, my updated documentation (as a separate patch, of course), or some combination of the two?
Comment 8 Jonathan Wakely 2012-05-29 23:21:23 UTC
I would keep the patches separate.
Comment 9 Jonathan Wakely 2014-04-27 12:19:05 UTC
(In reply to Jonathan Wakely from comment #3)
> > # Item 11: Define a copy constructor and an assignment operator for classes with
> > dynamically allocated memory.
> 
> Replaced by Item 14: "Think carefully about copying behavior in
> resource-managing classes" - the advice is less specific, but more useful. 
> I'm not sure how to turn it into a warning though!

I think this could be replaced by a new warning for PR 58407
Comment 10 Jonathan Wakely 2014-11-14 14:36:24 UTC
*** Bug 63871 has been marked as a duplicate of this bug. ***
Comment 11 Eric Gallager 2017-08-22 20:25:48 UTC
*** Bug 81431 has been marked as a duplicate of this bug. ***
Comment 12 Jonathan Wakely 2018-02-16 13:36:08 UTC
Created attachment 43443 [details]
Patch to split -Weffc++ into separate options.

This adds several new warning options to replace -Weffc++, and makes -Weffc++ enable them all.
Comment 13 Eric Gallager 2018-08-17 02:56:31 UTC
(In reply to Jonathan Wakely from comment #12)
> Created attachment 43443 [details]
> Patch to split -Weffc++ into separate options.
> 
> This adds several new warning options to replace -Weffc++, and makes
> -Weffc++ enable them all.

Right I've been meaning to try this patch but couldn't, due to having been blocked from bootstrapping for a while by bug 82092... it looks like that one might be getting fixed around now though, so once I've verified that, I'll try this...
Comment 14 Eric Gallager 2018-11-02 18:15:42 UTC
This came up on the gcc-help mailing list here: https://gcc.gnu.org/ml/gcc-help/2018-11/msg00003.html