* [patch] fix c++/18016 - warn about self-initialization in constructor init-list
@ 2010-12-21 16:06 Jonathan Wakely
2010-12-21 18:21 ` Paolo Carlini
2011-05-23 0:14 ` Jonathan Wakely
0 siblings, 2 replies; 8+ messages in thread
From: Jonathan Wakely @ 2010-12-21 16:06 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 593 bytes --]
Here's my attempt to fix PR c++/18016 so that we get a warning from
struct S {
int i;
S() : i(i) { }
};
-Winit-self is broken for C++ (PR c++/34772) so I made this warn with
-Wuninitialized. If you want to suppress the warning just don't put a
mem-initializer in the constructor (which will work at least until
someone fixes PR c++/2972)
cp/ChangeLog:
PR c++/18016
* init.c (perform_member_init): Check for self-initialization.
testsuite/ChangeLog:
PR c++/18016
* g++.dg/warn/pr18016.C: New.
tested x86_64-linux with no regressions, ok for trunk?
[-- Attachment #2: 18016.txt --]
[-- Type: text/plain, Size: 1272 bytes --]
Index: cp/init.c
===================================================================
--- cp/init.c (revision 168023)
+++ cp/init.c (working copy)
@@ -449,6 +449,17 @@ perform_member_init (tree member, tree i
if (decl == error_mark_node)
return;
+ if (warn_uninitialized && init && init != NULL_TREE
+ && TREE_CODE (init) == TREE_LIST && TREE_CHAIN (init) == NULL_TREE)
+ {
+ tree val = TREE_VALUE (init);
+ if (TREE_CODE (val) == COMPONENT_REF && TREE_OPERAND (val, 1) == member
+ && TREE_OPERAND (val, 0) == current_class_ref)
+ warning_at (DECL_SOURCE_LOCATION (current_function_decl),
+ OPT_Wuninitialized, "%qD is initialized with itself",
+ member);
+ }
+
if (init == void_type_node)
{
/* mem() means value-initialization. */
Index: testsuite/g++.dg/warn/pr18016.C
===================================================================
--- testsuite/g++.dg/warn/pr18016.C (revision 0)
+++ testsuite/g++.dg/warn/pr18016.C (revision 0)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-Wuninitialized" } */
+
+class X {
+ int i;
+ X() : i(i) { } // { dg-warning "initialized with itself" }
+ X(int i) : i(i) { }
+ X(const X& x) : i(x.i) { }
+};
+
+// { dg-prune-output "In constructor" }
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] fix c++/18016 - warn about self-initialization in constructor init-list
2010-12-21 16:06 [patch] fix c++/18016 - warn about self-initialization in constructor init-list Jonathan Wakely
@ 2010-12-21 18:21 ` Paolo Carlini
2010-12-21 18:35 ` Jonathan Wakely
2011-05-23 0:14 ` Jonathan Wakely
1 sibling, 1 reply; 8+ messages in thread
From: Paolo Carlini @ 2010-12-21 18:21 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: gcc-patches
Hi,
+ if (warn_uninitialized && init && init != NULL_TREE
I think the last check in this line is redundant.
Paolo.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] fix c++/18016 - warn about self-initialization in constructor init-list
2010-12-21 18:21 ` Paolo Carlini
@ 2010-12-21 18:35 ` Jonathan Wakely
2010-12-22 0:06 ` Jonathan Wakely
0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Wakely @ 2010-12-21 18:35 UTC (permalink / raw)
To: Paolo Carlini; +Cc: gcc-patches
On 21 December 2010 17:04, Paolo Carlini wrote:
> Hi,
>
> + if (warn_uninitialized && init && init != NULL_TREE
>
> I think the last check in this line is redundant.
Thanks, Paolo. I should have checked the definition of NULL_TREE.
I'm re-testing with that change, but it shouldn't affect the results.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] fix c++/18016 - warn about self-initialization in constructor init-list
2010-12-21 18:35 ` Jonathan Wakely
@ 2010-12-22 0:06 ` Jonathan Wakely
0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Wakely @ 2010-12-22 0:06 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 417 bytes --]
On 21 December 2010 17:11, Jonathan Wakely wrote:
> On 21 December 2010 17:04, Paolo Carlini wrote:
>> Hi,
>>
>> + if (warn_uninitialized && init && init != NULL_TREE
>>
>> I think the last check in this line is redundant.
>
> Thanks, Paolo. I should have checked the definition of NULL_TREE.
>
> I'm re-testing with that change, but it shouldn't affect the results.
Updated patch, tested x86_64-linux
[-- Attachment #2: 18016-2.txt --]
[-- Type: text/plain, Size: 1251 bytes --]
Index: cp/init.c
===================================================================
--- cp/init.c (revision 168023)
+++ cp/init.c (working copy)
@@ -449,6 +449,17 @@ perform_member_init (tree member, tree i
if (decl == error_mark_node)
return;
+ if (warn_uninitialized && init && TREE_CODE (init) == TREE_LIST
+ && TREE_CHAIN (init) == NULL_TREE)
+ {
+ tree val = TREE_VALUE (init);
+ if (TREE_CODE (val) == COMPONENT_REF && TREE_OPERAND (val, 1) == member
+ && TREE_OPERAND (val, 0) == current_class_ref)
+ warning_at (DECL_SOURCE_LOCATION (current_function_decl),
+ OPT_Wuninitialized, "%qD is initialized with itself",
+ member);
+ }
+
if (init == void_type_node)
{
/* mem() means value-initialization. */
Index: testsuite/g++.dg/warn/pr18016.C
===================================================================
--- testsuite/g++.dg/warn/pr18016.C (revision 0)
+++ testsuite/g++.dg/warn/pr18016.C (revision 0)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-Wuninitialized" } */
+
+class X {
+ int i;
+ X() : i(i) { } // { dg-warning "initialized with itself" }
+ X(int i) : i(i) { }
+ X(const X& x) : i(x.i) { }
+};
+
+// { dg-prune-output "In constructor" }
^ permalink raw reply [flat|nested] 8+ messages in thread
* [patch] fix c++/18016 - warn about self-initialization in constructor init-list
2010-12-21 16:06 [patch] fix c++/18016 - warn about self-initialization in constructor init-list Jonathan Wakely
2010-12-21 18:21 ` Paolo Carlini
@ 2011-05-23 0:14 ` Jonathan Wakely
2011-05-23 9:26 ` Jason Merrill
1 sibling, 1 reply; 8+ messages in thread
From: Jonathan Wakely @ 2011-05-23 0:14 UTC (permalink / raw)
To: gcc-patches; +Cc: Jason Merrill
[-- Attachment #1: Type: text/plain, Size: 937 bytes --]
I didn't get this reviewed previously so I'll try again for 4.7 ...
---------- Forwarded message ----------
Here's my attempt to fix PR c++/18016 so that we get a warning from
struct S {
int i;
S() : i(i) { }
};
As I said in the audit trail, -Winit-self is broken for C++ (PR
c++/34772) so I made this warn with
-Wuninitialized and not affected by -Winit-self.
Even if -Winit-self wasn't broken I don't think it applies here,
there's no valid reason to initialize a member with itself, if you
really want to leave a member uninitialized then don't use a
mem-initializer in the constructor (which will work at least until
someone fixes PR c++/2972)
cp/ChangeLog:
PR c++/18016
* init.c (perform_member_init): Check for self-initialization.
testsuite/ChangeLog:
PR c++/18016
* g++.dg/warn/pr18016.C: New.
tested x86_64-linux with no regressions, ok for trunk?
[-- Attachment #2: 18016-3.txt --]
[-- Type: text/plain, Size: 1203 bytes --]
Index: cp/init.c
===================================================================
--- cp/init.c (revision 174044)
+++ cp/init.c (working copy)
@@ -501,6 +501,17 @@
if (decl == error_mark_node)
return;
+ if (warn_uninitialized && init && TREE_CODE (init) == TREE_LIST
+ && TREE_CHAIN (init) == NULL_TREE)
+ {
+ tree val = TREE_VALUE (init);
+ if (TREE_CODE (val) == COMPONENT_REF && TREE_OPERAND (val, 1) == member
+ && TREE_OPERAND (val, 0) == current_class_ref)
+ warning_at (DECL_SOURCE_LOCATION (current_function_decl),
+ OPT_Wuninitialized, "%qD is initialized with itself",
+ member);
+ }
+
if (init == void_type_node)
{
/* mem() means value-initialization. */
Index: testsuite/g++.dg/warn/pr18016.C
===================================================================
--- testsuite/g++.dg/warn/pr18016.C (revision 0)
+++ testsuite/g++.dg/warn/pr18016.C (revision 0)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-Wuninitialized" } */
+
+class X {
+ int i;
+ X() : i(i) { } // { dg-warning "initialized with itself" }
+ X(int i) : i(i) { }
+ X(const X& x) : i(x.i) { }
+};
+
+// { dg-prune-output "In constructor" }
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] fix c++/18016 - warn about self-initialization in constructor init-list
2011-05-23 0:14 ` Jonathan Wakely
@ 2011-05-23 9:26 ` Jason Merrill
2011-05-23 9:47 ` Jonathan Wakely
0 siblings, 1 reply; 8+ messages in thread
From: Jason Merrill @ 2011-05-23 9:26 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: gcc-patches
On 05/22/2011 05:11 PM, Jonathan Wakely wrote:
> As I said in the audit trail, -Winit-self is broken for C++ (PR
> c++/34772)
Not anymore. :)
> Even if -Winit-self wasn't broken I don't think it applies here,
> there's no valid reason to initialize a member with itself, if you
> really want to leave a member uninitialized then don't use a
> mem-initializer in the constructor (which will work at least until
> someone fixes PR c++/2972)
...at which point we would want it to be controlled by -Winit-self, so
let's use -Winit-self now.
OK with that change.
Jason
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] fix c++/18016 - warn about self-initialization in constructor init-list
2011-05-23 9:26 ` Jason Merrill
@ 2011-05-23 9:47 ` Jonathan Wakely
2011-05-23 14:59 ` Jason Merrill
0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Wakely @ 2011-05-23 9:47 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]
On 23 May 2011 03:07, Jason Merrill wrote:
> On 05/22/2011 05:11 PM, Jonathan Wakely wrote:
>>
>> As I said in the audit trail, -Winit-self is broken for C++ (PR
>> c++/34772)
>
> Not anymore. :)
Aha!
>> Even if -Winit-self wasn't broken I don't think it applies here,
>> there's no valid reason to initialize a member with itself, if you
>> really want to leave a member uninitialized then don't use a
>> mem-initializer in the constructor (which will work at least until
>> someone fixes PR c++/2972)
>
> ...at which point we would want it to be controlled by -Winit-self, so let's
> use -Winit-self now.
I still think this warning shouldn't be tied to -Winit-self. That
isn't included in -Wall or -Wextra, and it implies we're saying that a
self-referencing mem-initializer is a GNU extension to silence a
warning, when I think it's always a mistake and never done
intentionally. But I can live with that.
> OK with that change.
Thanks, I've attached what I committed. I only checked
warn_init_self, not both warn_uninitialized && warn_init_self, I think
that's right.
I've also just noticed that my change doesn't catch references which
are initialized with themselves. I won't try to fix that now, but
would that be done by checking for INDIRECT_REF?
I also had some thoughts about making init-self work for class types e.g.
std::string s = s;
Would checking if the copy constructor's parameter is *this be a
simple way to catch that?
[-- Attachment #2: 18016-4.txt --]
[-- Type: text/plain, Size: 1211 bytes --]
Index: cp/init.c
===================================================================
--- cp/init.c (revision 174044)
+++ cp/init.c (working copy)
@@ -501,6 +501,17 @@
if (decl == error_mark_node)
return;
+ if (warn_init_self && init && TREE_CODE (init) == TREE_LIST
+ && TREE_CHAIN (init) == NULL_TREE)
+ {
+ tree val = TREE_VALUE (init);
+ if (TREE_CODE (val) == COMPONENT_REF && TREE_OPERAND (val, 1) == member
+ && TREE_OPERAND (val, 0) == current_class_ref)
+ warning_at (DECL_SOURCE_LOCATION (current_function_decl),
+ OPT_Wuninitialized, "%qD is initialized with itself",
+ member);
+ }
+
if (init == void_type_node)
{
/* mem() means value-initialization. */
Index: testsuite/g++.dg/warn/pr18016.C
===================================================================
--- testsuite/g++.dg/warn/pr18016.C (revision 0)
+++ testsuite/g++.dg/warn/pr18016.C (revision 0)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-Wuninitialized -Winit-self" } */
+
+class X {
+ int i;
+ X() : i(i) { } // { dg-warning "initialized with itself" }
+ X(int i) : i(i) { }
+ X(const X& x) : i(x.i) { }
+};
+
+// { dg-prune-output "In constructor" }
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] fix c++/18016 - warn about self-initialization in constructor init-list
2011-05-23 9:47 ` Jonathan Wakely
@ 2011-05-23 14:59 ` Jason Merrill
0 siblings, 0 replies; 8+ messages in thread
From: Jason Merrill @ 2011-05-23 14:59 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: gcc-patches
On 05/23/2011 04:20 AM, Jonathan Wakely wrote:
> I still think this warning shouldn't be tied to -Winit-self. That
> isn't included in -Wall or -Wextra, and it implies we're saying that a
> self-referencing mem-initializer is a GNU extension to silence a
> warning, when I think it's always a mistake and never done
> intentionally.
I agree, but I think the answer to that is to enable -Winit-self with
-Wextra in C++ mode.
> Thanks, I've attached what I committed. I only checked
> warn_init_self, not both warn_uninitialized&& warn_init_self, I think
> that's right.
I think so.
> I've also just noticed that my change doesn't catch references which
> are initialized with themselves. I won't try to fix that now, but
> would that be done by checking for INDIRECT_REF?
Yes, or you can use REFERENCE_REF_P.
> I also had some thoughts about making init-self work for class types e.g.
> std::string s = s;
> Would checking if the copy constructor's parameter is *this be a
> simple way to catch that?
Well, in this snippet there's no *this involved, the parameter would be
the decl being initialized.
Jason
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-05-23 14:19 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-21 16:06 [patch] fix c++/18016 - warn about self-initialization in constructor init-list Jonathan Wakely
2010-12-21 18:21 ` Paolo Carlini
2010-12-21 18:35 ` Jonathan Wakely
2010-12-22 0:06 ` Jonathan Wakely
2011-05-23 0:14 ` Jonathan Wakely
2011-05-23 9:26 ` Jason Merrill
2011-05-23 9:47 ` Jonathan Wakely
2011-05-23 14:59 ` Jason Merrill
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).