[Bug c++/16166] -Weffc++ finer granularity

[Bug c++/16166] -Weffc++ finer granularity

[redi at gcc dot gnu.org]

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

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> 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.

[Bug c++/16166] -Weffc++ finer granularity

[david at doublewise dot net]

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

--- Comment #4 from David Stone <david at doublewise dot net> 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.

[Bug c++/16166] -Weffc++ finer granularity

[redi at gcc dot gnu.org]

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

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> 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

[Bug c++/16166] -Weffc++ finer granularity

[manu at gcc dot gnu.org]

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

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |manu at gcc dot gnu.org

--- Comment #6 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 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!

[Bug c++/16166] -Weffc++ finer granularity

[david at doublewise dot net]

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

--- Comment #7 from David Stone <david at doublewise dot net> 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?

[Bug c++/16166] -Weffc++ finer granularity

[redi at gcc dot gnu.org]

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

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-05-29 23:21:23 UTC ---
I would keep the patches separate.

[Bug c++/16166] -Weffc++ finer granularity

[redi at gcc dot gnu.org]

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

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(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

[Bug c++/16166] -Weffc++ finer granularity

[pinskia at gcc dot gnu dot org]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic
   Last reconfirmed|2004-10-30 17:49:38         |2005-01-29 03:19:30
               date|                            |


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

[Bug c++/16166] -Weffc++ finer granularity

[lerdsuwa at gcc dot gnu dot org]

------- Additional Comments From lerdsuwa at gcc dot gnu dot org  2004-07-31 12:16 -------
Confirmed.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|                            |1
   Last reconfirmed|0000-00-00 00:00:00         |2004-07-31 12:16:53
               date|                            |


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

[Bug c++/16166] -Weffc++ finer granularity

[bkoz at gcc dot gnu dot org]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
OtherBugsDependingO|                            |12854
              nThis|                            |


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

[Bug c++/16166] -Weffc++ finer granularity

[bkoz at gcc dot gnu dot org]

------- Additional Comments From bkoz at gcc dot gnu dot org  2004-06-24 02:08 -------

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.



-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|-Weffc++ finer granularity  |-Weffc++ finer granularity


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