| Message ID | CAFH4-diup6eH5j3MhZcknAAia+6ZSyzwD5D94WV0NMa+otgjFg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Ping (yeah boring to review!) 2014-02-23 20:36 GMT+01:00 Fabien Chêne <fabien.chene@gmail.com>: > Ahem, patch resubmitted with the testsuite correctly adjusted this > time. Tested x86_64 linux, still OK to commit ? > > 2014-02-23 Fabien Chene <fabien@gcc.gnu.org> > PR c++/52369 > * cp/method.c (walk_field_subobs): improve the diagnostic > locations for both REFERENCE_TYPEs and non-static const members. > > 2014-02-23 Fabien Chene <fabien@gcc.gnu.org> > > PR c++/52369 > * g++.dg/init/const10.C: New. > * g++.dg/init/const11.C: New. > * g++.dg/init/pr25811.C: Adjust. > * g++.dg/init/pr29043.C: Adjust. > * g++.dg/init/pr43719.C: Likewise. > * g++.dg/init/pr44086.C: Likewise. > * g++.dg/init/ctor4.C: Likewise. > * g++.dg/init/ctor8.C: Likewise. > * g++.dg/init/uninitialized1.C: Likewise.
On 02/23/2014 02:36 PM, Fabien Chêne wrote: > * cp/method.c (walk_field_subobs): improve the diagnostic > locations for both REFERENCE_TYPEs and non-static const members. It's important to have the error location be the place where the actual problem is, namely the constructor definition. Instead of changing it, please add an inform pointing out the location of the member in question. Jason
2014-02-27 19:25 GMT+01:00 Jason Merrill <jason@redhat.com>: > On 02/23/2014 02:36 PM, Fabien Chêne wrote: >> >> * cp/method.c (walk_field_subobs): improve the diagnostic >> locations for both REFERENCE_TYPEs and non-static const members. > > > It's important to have the error location be the place where the actual > problem is, namely the constructor definition. Instead of changing it, > please add an inform pointing out the location of the member in question. Well, I am not very happy with the c++11 diagnostic compared to the c++98 one. Below is the original c++11 diagnostic for pr44086.C: struct A { int const i : 2; }; void f() { A a; // { dg-error "deleted|uninitialized const" } new A; // { dg-error "deleted|uninitialized const" } A(); // { dg-error "deleted" "" { target c++11 } } new A(); // { dg-error "deleted" "" { target c++11 } } } testsuite/g++.dg/init/uninitialized1.C:10:3: error: use of deleted function 'A::A()' testsuite/g++.dg/init/uninitialized1.C:3:8: note: 'A::A()' is implicitly deleted because the default definition would be ill-formed: testsuite/g++.dg/init/uninitialized1.C:3:8: error: uninitialized non-static const member 'const int A::value1' The first two lines are fine in my opinion. The third line should actually be split into an error + an inform. By doing that, I think we also need to reformulate the error message like this: testsuite/g++.dg/init/pr44086.C:4:8: error: 'struct A' needs its non-static const members to be initialized testsuite/g++.dg/init/pr44086.C:6:19: note: 'A::i' should be initialized What do you think ? (before I bother adjusting the testsuite) Incidentally, while moving the diagnostic concerning the uninitialized field from an error to an inform, I realized that the syntactic sugar %q#D is no longer honored an is treated as %qD, is it expected ?
On 02/28/2014 04:03 PM, Fabien Chêne wrote: > The first two lines are fine in my opinion. The third line should > actually be split into an error + an inform. By doing that, I think we > also need to reformulate the error message like this: > testsuite/g++.dg/init/pr44086.C:4:8: error: 'struct A' needs its > non-static const members to be initialized > testsuite/g++.dg/init/pr44086.C:6:19: note: 'A::i' should be initialized > > What do you think ? (before I bother adjusting the testsuite) Let's change the C++11 diagnostic to match the C++98 diagnostic. So, "uninitialized const member in %q#T" + "%qD should be initialized". > Incidentally, while moving the diagnostic concerning the uninitialized > field from an error to an inform, I realized that the syntactic sugar > %q#D is no longer honored an is treated as %qD, is it expected ? No, how do you mean? Jason
2014-02-28 22:27 GMT+01:00 Jason Merrill <jason@redhat.com>: > Let's change the C++11 diagnostic to match the C++98 diagnostic. So, > "uninitialized const member in %q#T" + "%qD should be initialized". OK. >> Incidentally, while moving the diagnostic concerning the uninitialized >> field from an error to an inform, I realized that the syntactic sugar >> %q#D is no longer honored an is treated as %qD, is it expected ? > > > No, how do you mean? I must be tired, false alarm, sorry.
2014-02-28 22:52 GMT+01:00 Fabien Chêne <fabien.chene@gmail.com>: >>> Incidentally, while moving the diagnostic concerning the uninitialized >>> field from an error to an inform, I realized that the syntactic sugar >>> %q#D is no longer honored an is treated as %qD, is it expected ? >> >> >> No, how do you mean? > > I must be tired, false alarm, sorry. I guess my mistake comes from the fact that %q#D is not present in the c++98 diagnostic. Shall we homogeneise that as well ? In favor of %q#D ?
On 02/28/2014 05:04 PM, Fabien Chêne wrote: > I guess my mistake comes from the fact that %q#D is not present in the > c++98 diagnostic. Shall we homogeneise that as well ? > In favor of %q#D ? OK. Jason
2014-02-28 22:52 GMT+01:00 Fabien Chêne <fabien.chene@gmail.com>: > 2014-02-28 22:27 GMT+01:00 Jason Merrill <jason@redhat.com>: >> Let's change the C++11 diagnostic to match the C++98 diagnostic. So, >> "uninitialized const member in %q#T" + "%qD should be initialized". > > OK. Hmm, sorry to iterate on this rather trivial issue, but it seems difficult to mimic the c++98 diagnostic. Actually, the c++98 diagnosic raises an error at the point of use, mention the class implied, and add a note at the ref/const member uninitialized. Doing that in c++11 is not currently possible because input_location is sabotaged early in maybe_explain_implicit_delete (unless there is some magic incantation in the diagnostic machinery I am not aware of), and the point of use is lost. Reading the comment of synthesized_method_walk, which is the only caller of walk_field_subobs: "If diag is true, we're either being called from maybe_explain_implicit_delete to give errors, or if constexpr_p is non-null, from explain_invalid_constexpr_fn." I am inclined to think that if we reached one the two functions mentioned, an error had already been raised and we are trying to explain why. Thus, it seems to me that only notes should be emitted. Here we are actually explaining why the default constructor is deleted, which is a kind of subnote. Hence, I would say that emitting an error at this point would be seen as a different issue by the user. All in all, in my opinion, just emitting a note would be appropriate given the context. If agreed, the comment above should be adjusted, and says "... called from maybe_explain_implicit_delete to ***emit additional notes***...", and the errors raised in callees shall be turned into notes. Unfortunalely, the comment does not match the code and synthesized_method_walk is called with diag=true in another context: /* Warn about calling a non-trivial move assignment in a virtual base. */ if (kind == sfk_move_assignment && !deleted_p && !trivial_p && CLASSTYPE_VBASECLASSES (type)) { location_t loc = input_location; input_location = DECL_SOURCE_LOCATION (fn); synthesized_method_walk (type, kind, const_p, NULL, NULL, NULL, NULL, true, NULL_TREE, NULL_TREE); input_location = loc; } ... Which breaks the logic above, even if it probably intends to reach this warning later: warning (OPT_Wvirtual_move_assign, "defaulted move assignment for %qT calls a non-trivial " "move assignment operator for virtual base %qT", ctype, basetype); This can of revamping I suggest can wait for next stage 1, obviously... What do you think ?
On 03/02/2014 04:55 PM, Fabien Chêne wrote: > Hmm, sorry to iterate on this rather trivial issue, but it seems > difficult to mimic the c++98 diagnostic. Actually, the c++98 diagnosic > raises an error at the point of use, mention the class implied, and > add a note at the ref/const member uninitialized. Doing that in c++11 > is not currently possible because input_location is sabotaged early in > maybe_explain_implicit_delete (unless there is some magic incantation > in the diagnostic machinery I am not aware of), and the point of use > is lost. Yes, in C++11 the point of use is the source location of the constructor, which is going to be different from where the constructor is called. I just meant use the same wording. > I am inclined to think that if we reached one the two functions > mentioned, an error had already been raised and we are trying to > explain why. Thus, it seems to me that only notes should be emitted. > Here we are actually explaining why the default constructor is > deleted, which is a kind of subnote. I can see that argument, but it's deleted because if it were defined, that definition would be ill-formed, and we're giving the errors that we would see in that case. Jason
OK, thanks. Jason
Index: gcc/testsuite/g++.dg/init/const10.C =================================================================== --- gcc/testsuite/g++.dg/init/const10.C (revision 0) +++ gcc/testsuite/g++.dg/init/const10.C (revision 0) @@ -0,0 +1,31 @@ +// PR C++/52369 +// { dg-do compile { target c++11 } } + +class B // { dg-message "implicitly deleted" } +{ + int const v_; // { dg-error "uninitialized" } +}; + +struct D : B {}; // { dg-error "deleted" } + +class A // { dg-message "implicitly deleted" } +{ + int& ref; // { dg-error "uninitialized" } +}; + +struct C : A {}; // { dg-error "deleted" } + +void f() +{ + D d; // { dg-error "use of deleted" } + new D; // { dg-error "use of deleted" } + D(); // { dg-error "use of deleted" } + new D(); // { dg-error "use of deleted" } + + C c; // { dg-error "use of deleted" } + new C; // { dg-error "use of deleted" } + C(); // { dg-error "use of deleted" } + new C(); // { dg-error "use of deleted" } +} + + Index: gcc/testsuite/g++.dg/init/const11.C =================================================================== --- gcc/testsuite/g++.dg/init/const11.C (revision 0) +++ gcc/testsuite/g++.dg/init/const11.C (revision 0) @@ -0,0 +1,29 @@ +// PR C++/52369 +// { dg-do compile { target c++98 } } + +class B +{ + int const v_; // { dg-message "should be initialized" } +}; + +struct D : B {}; + +class A +{ + int& ref; // { dg-message "should be initialized" } +}; + +struct C : A {}; + +void f() +{ + D d; // { dg-error "uninitialized" } + new D; // { dg-error "uninitialized" } + D(); + new D(); + + C c; // { dg-error "uninitialized" } + new C; // { dg-error "uninitialized" } + C(); // { dg-error "value-initialization" } + new C(); // { dg-error "value-initialization" } +} Index: gcc/cp/method.c =================================================================== --- gcc/cp/method.c (revision 207406) +++ gcc/cp/method.c (working copy) @@ -1091,15 +1091,15 @@ walk_field_subobs (tree fields, tree fnn && default_init_uninitialized_part (mem_type)) { if (diag) - error ("uninitialized non-static const member %q#D", - field); + error_at (DECL_SOURCE_LOCATION (field), + "uninitialized non-static const member %q#D", field); bad = true; } else if (TREE_CODE (mem_type) == REFERENCE_TYPE) { if (diag) - error ("uninitialized non-static reference member %q#D", - field); + error_at (DECL_SOURCE_LOCATION (field), + "uninitialized non-static reference member %q#D", field); bad = true; }
Hi, This bug seems already fixed on mainline. I have added two testcases, but as the C++11 one is not obvious to me, I guess this path does not qualify as obvious. Incidentally, while writing the C++11 testcase, I realized that the diagnostic was not emitted at the proper location and fixed it. OK for trunk ? 2014-02-06 Fabien Chene <fabien@gcc.gnu.org> PR c++/52369 * cp/method.c (walk_field_subobs): improve the diagnostic locations for both REFERENCE_TYPEs and non-static const members. 2014-02-06 Fabien Chene <fabien@gcc.gnu.org> PR c++/52369 * g++.dg/init/const10.C: New. * g++.dg/init/const11.C: New.