* [Bug c++/16166] -Weffc++ finer granularity
[not found] <bug-16166-4@http.gcc.gnu.org/bugzilla/>
@ 2011-11-07 16:14 ` redi at gcc dot gnu.org
2012-05-29 2:39 ` david at doublewise dot net
` (5 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: redi at gcc dot gnu.org @ 2011-11-07 16:14 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* [Bug c++/16166] -Weffc++ finer granularity
[not found] <bug-16166-4@http.gcc.gnu.org/bugzilla/>
2011-11-07 16:14 ` redi at gcc dot gnu.org
@ 2012-05-29 2:39 ` david at doublewise dot net
2012-05-29 9:07 ` redi at gcc dot gnu.org
` (4 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: david at doublewise dot net @ 2012-05-29 2:39 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* [Bug c++/16166] -Weffc++ finer granularity
[not found] <bug-16166-4@http.gcc.gnu.org/bugzilla/>
2011-11-07 16:14 ` redi at gcc dot gnu.org
2012-05-29 2:39 ` david at doublewise dot net
@ 2012-05-29 9:07 ` redi at gcc dot gnu.org
2012-05-29 10:03 ` manu at gcc dot gnu.org
` (3 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: redi at gcc dot gnu.org @ 2012-05-29 9:07 UTC (permalink / raw)
To: gcc-bugs
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [Bug c++/16166] -Weffc++ finer granularity
[not found] <bug-16166-4@http.gcc.gnu.org/bugzilla/>
` (2 preceding siblings ...)
2012-05-29 9:07 ` redi at gcc dot gnu.org
@ 2012-05-29 10:03 ` manu at gcc dot gnu.org
2012-05-29 21:01 ` david at doublewise dot net
` (2 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: manu at gcc dot gnu.org @ 2012-05-29 10:03 UTC (permalink / raw)
To: gcc-bugs
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!
^ permalink raw reply [flat|nested] 12+ messages in thread* [Bug c++/16166] -Weffc++ finer granularity
[not found] <bug-16166-4@http.gcc.gnu.org/bugzilla/>
` (3 preceding siblings ...)
2012-05-29 10:03 ` manu at gcc dot gnu.org
@ 2012-05-29 21:01 ` david at doublewise dot net
2012-05-29 23:34 ` redi at gcc dot gnu.org
2014-04-27 12:19 ` redi at gcc dot gnu.org
6 siblings, 0 replies; 12+ messages in thread
From: david at doublewise dot net @ 2012-05-29 21:01 UTC (permalink / raw)
To: gcc-bugs
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?
^ permalink raw reply [flat|nested] 12+ messages in thread* [Bug c++/16166] -Weffc++ finer granularity
[not found] <bug-16166-4@http.gcc.gnu.org/bugzilla/>
` (4 preceding siblings ...)
2012-05-29 21:01 ` david at doublewise dot net
@ 2012-05-29 23:34 ` redi at gcc dot gnu.org
2014-04-27 12:19 ` redi at gcc dot gnu.org
6 siblings, 0 replies; 12+ messages in thread
From: redi at gcc dot gnu.org @ 2012-05-29 23:34 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* [Bug c++/16166] -Weffc++ finer granularity
[not found] <bug-16166-4@http.gcc.gnu.org/bugzilla/>
` (5 preceding siblings ...)
2012-05-29 23:34 ` redi at gcc dot gnu.org
@ 2014-04-27 12:19 ` redi at gcc dot gnu.org
6 siblings, 0 replies; 12+ messages in thread
From: redi at gcc dot gnu.org @ 2014-04-27 12:19 UTC (permalink / raw)
To: gcc-bugs
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
^ permalink raw reply [flat|nested] 12+ messages in thread