| Message ID | f9ea299f-5867-a6be-56e1-88024dd9e428@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] Three additional bitfield diagnostic tweaks (a regression fix included) | expand |
On 12/6/18 5:49 AM, Paolo Carlini wrote: > Hi, > > I'm bundling together 3 more. In attachment order: > > 1- Since we decided to explicitly print the wrong type, I suppose we > want to consistently do that in templates too. OK. > 2- Unfortunately I have to fix another buglet I recently introduced, > completely similar to c++/88222 fixed by Marek. Well, at least we will > not print anymore an empty '' when the unqualified_id is null because > the field is unnamed. > - error_at (declarator->id_loc, > - "%qE is neither function nor member function; " > - "cannot be declared friend", unqualified_id); > + if (unqualified_id && declarator) > + error_at (declarator->id_loc, > + "%qE is neither function nor member function; " > + "cannot be declared friend", unqualified_id); > + else > + error ("unnamed field is neither function nor member " > + "function; cannot be declared friend"); I wonder if we want to use the 'name' variable here. > 3- In the non-static case too, when from grokdeclarator we are calling > FIELD_DECL and passing the location as first argument, I think we want > to likewise pass declarator->id_loc when available. > - decl = build_decl (input_location, > + decl = build_decl (declarator > + ? declarator->id_loc > + : input_location, I think we want to put this in a local variable, to share with the static case and probably other places in grokdeclarator. Jason
Hi, On 06/12/18 16:11, Jason Merrill wrote: > 2- Unfortunately I have to fix another buglet I recently introduced, > completely similar to c++/88222 fixed by Marek. Well, at least we will > not print anymore an empty '' when the unqualified_id is null because > the field is unnamed. > >> - error_at (declarator->id_loc, >> - "%qE is neither function nor member function; " >> - "cannot be declared friend", unqualified_id); >> + if (unqualified_id && declarator) >> + error_at (declarator->id_loc, >> + "%qE is neither function nor member function; " >> + "cannot be declared friend", unqualified_id); >> + else >> + error ("unnamed field is neither function nor member " >> + "function; cannot be declared friend"); > > I wonder if we want to use the 'name' variable here. Well, the name variable doesn't seem that useful here because for the new testcase it has that famous catch all value "type name" . I have been thinking that here and in other places we could imagine keeping only the declarator check and dropping the "name" check. Probably it would work. But in *many* existing places we actually check *only* the name thus I'm nervous about attempting that now... > >> 3- In the non-static case too, when from grokdeclarator we are >> calling FIELD_DECL and passing the location as first argument, I >> think we want to likewise pass declarator->id_loc when available. > >> - decl = build_decl (input_location, >> + decl = build_decl (declarator >> + ? declarator->id_loc >> + : input_location, > > I think we want to put this in a local variable, to share with the > static case and probably other places in grokdeclarator. In the below I'm sharing it only with the static case, straightforward. Moving it one level up doesn't seem that useful because we only have rather safe IMHO unconditional uses either of input_location or of declarator->id_loc at the moment... Again, I'm pretty sure there is room for further clean-ups in this area, but, for 9, I'd rather take care of a bunch of additional small issues which I already have in my TODO list, in grokbitfield, for example, as already mentioned. By the way, if isn't already clear, I have been changing location bits only when I already have a set of testcases, constructed from our testsuite via (lenghty ;) instrumented runs. New version of the patch attached. Paolo. Index: cp/class.c =================================================================== --- cp/class.c (revision 266840) +++ cp/class.c (working copy) @@ -3218,7 +3218,8 @@ check_bitfield_decl (tree field) /* Detect invalid bit-field type. */ if (!INTEGRAL_OR_ENUMERATION_TYPE_P (type)) { - error ("bit-field %q+#D with non-integral type", field); + error_at (DECL_SOURCE_LOCATION (field), + "bit-field %q#D with non-integral type %qT", field, type); w = error_mark_node; } else Index: cp/decl.c =================================================================== --- cp/decl.c (revision 266840) +++ cp/decl.c (working copy) @@ -12446,9 +12446,13 @@ grokdeclarator (const cp_declarator *declarator, { if (friendp) { - error_at (declarator->id_loc, - "%qE is neither function nor member function; " - "cannot be declared friend", unqualified_id); + if (unqualified_id && declarator) + error_at (declarator->id_loc, + "%qE is neither function nor member function; " + "cannot be declared friend", unqualified_id); + else + error ("unnamed field is neither function nor member " + "function; cannot be declared friend"); return error_mark_node; } decl = NULL_TREE; @@ -12483,14 +12487,13 @@ grokdeclarator (const cp_declarator *declarator, if (decl == NULL_TREE) { + location_t loc = declarator ? declarator->id_loc : input_location; if (staticp) { /* C++ allows static class members. All other work for this is done by grokfield. */ - decl = build_lang_decl_loc (declarator - ? declarator->id_loc - : input_location, - VAR_DECL, unqualified_id, type); + decl = build_lang_decl_loc (loc, VAR_DECL, + unqualified_id, type); set_linkage_for_static_data_member (decl); if (concept_p) error_at (declspecs->locations[ds_concept], @@ -12536,8 +12539,7 @@ grokdeclarator (const cp_declarator *declarator, unqualified_id); constexpr_p = false; } - decl = build_decl (input_location, - FIELD_DECL, unqualified_id, type); + decl = build_decl (loc, FIELD_DECL, unqualified_id, type); DECL_NONADDRESSABLE_P (decl) = bitfield; if (bitfield && !unqualified_id) { Index: testsuite/g++.dg/other/bitfield2.C =================================================================== --- testsuite/g++.dg/other/bitfield2.C (revision 266840) +++ testsuite/g++.dg/other/bitfield2.C (working copy) @@ -3,7 +3,7 @@ struct A { - double d : 2; // { dg-error "non-integral" } + double d : 2; // { dg-error "10:bit-field .d. with non-integral type .double." } A() {} ~A() {} }; Index: testsuite/g++.dg/parse/bitfield1.C =================================================================== --- testsuite/g++.dg/parse/bitfield1.C (revision 266840) +++ testsuite/g++.dg/parse/bitfield1.C (working copy) @@ -2,7 +2,7 @@ struct A { - double i : 8; // { dg-error "type" } + double i : 8; // { dg-error "10:bit-field .i. with non-integral type .double." } }; void foo(A& a) Index: testsuite/g++.dg/parse/bitfield2.C =================================================================== --- testsuite/g++.dg/parse/bitfield2.C (revision 266840) +++ testsuite/g++.dg/parse/bitfield2.C (working copy) @@ -4,7 +4,7 @@ struct X {}; struct A { - X x : 2; // { dg-error "non-integral type" } + X x : 2; // { dg-error "7:bit-field .x. with non-integral type .X." } }; struct B : A {}; @@ -19,7 +19,7 @@ C<int> c; template <typename T> struct D { - T t : 3; // { dg-error "non-integral type" } + T t : 3; // { dg-error "5:bit-field .double D\\<double\\>::t. with non-integral type .double." } }; D<double> d; // { dg-message "required" } @@ -28,7 +28,7 @@ template <typename T> struct E { typedef T* U; - U t : 3; // { dg-error "non-integral type" } + U t : 3; // { dg-error "5:bit-field .t. with non-integral type .E\\<T\\>::U." } }; E<double> e; Index: testsuite/g++.dg/parse/bitfield7.C =================================================================== --- testsuite/g++.dg/parse/bitfield7.C (nonexistent) +++ testsuite/g++.dg/parse/bitfield7.C (working copy) @@ -0,0 +1,4 @@ +struct A +{ + friend int : 1; // { dg-error "unnamed field" } +};
On 12/6/18 12:23 PM, Paolo Carlini wrote: > Hi, > > On 06/12/18 16:11, Jason Merrill wrote: >> 2- Unfortunately I have to fix another buglet I recently introduced, >> completely similar to c++/88222 fixed by Marek. Well, at least we will >> not print anymore an empty '' when the unqualified_id is null because >> the field is unnamed. >> >>> - error_at (declarator->id_loc, >>> - "%qE is neither function nor member function; " >>> - "cannot be declared friend", unqualified_id); >>> + if (unqualified_id && declarator) >>> + error_at (declarator->id_loc, >>> + "%qE is neither function nor member function; " >>> + "cannot be declared friend", unqualified_id); >>> + else >>> + error ("unnamed field is neither function nor member " >>> + "function; cannot be declared friend"); >> >> I wonder if we want to use the 'name' variable here. > > Well, the name variable doesn't seem that useful here because for the > new testcase it has that famous catch all value "type name" . > > I have been thinking that here and in other places we could imagine > keeping only the declarator check and dropping the "name" check. > Probably it would work. But in *many* existing places we actually check > *only* the name thus I'm nervous about attempting that now... > >> >>> 3- In the non-static case too, when from grokdeclarator we are >>> calling FIELD_DECL and passing the location as first argument, I >>> think we want to likewise pass declarator->id_loc when available. >> >>> - decl = build_decl (input_location, >>> + decl = build_decl (declarator >>> + ? declarator->id_loc >>> + : input_location, >> >> I think we want to put this in a local variable, to share with the >> static case and probably other places in grokdeclarator. > > In the below I'm sharing it only with the static case, straightforward. > Moving it one level up doesn't seem that useful because we only have > rather safe IMHO unconditional uses either of input_location or of > declarator->id_loc at the moment... Again, I'm pretty sure there is room > for further clean-ups in this area, but, for 9, I'd rather take care of > a bunch of additional small issues which I already have in my TODO list, > in grokbitfield, for example, as already mentioned. By the way, if isn't > already clear, I have been changing location bits only when I already > have a set of testcases, constructed from our testsuite via (lenghty ;) > instrumented runs. > > New version of the patch attached. OK. Jason
Index: cp/class.c =================================================================== --- cp/class.c (revision 266840) +++ cp/class.c (working copy) @@ -3218,7 +3218,8 @@ check_bitfield_decl (tree field) /* Detect invalid bit-field type. */ if (!INTEGRAL_OR_ENUMERATION_TYPE_P (type)) { - error ("bit-field %q+#D with non-integral type", field); + error_at (DECL_SOURCE_LOCATION (field), + "bit-field %q#D with non-integral type %qT", field, type); w = error_mark_node; } else Index: cp/decl.c =================================================================== --- cp/decl.c (revision 266840) +++ cp/decl.c (working copy) @@ -12446,9 +12446,13 @@ grokdeclarator (const cp_declarator *declarator, { if (friendp) { - error_at (declarator->id_loc, - "%qE is neither function nor member function; " - "cannot be declared friend", unqualified_id); + if (unqualified_id && declarator) + error_at (declarator->id_loc, + "%qE is neither function nor member function; " + "cannot be declared friend", unqualified_id); + else + error ("unnamed field is neither function nor member " + "function; cannot be declared friend"); return error_mark_node; } decl = NULL_TREE; @@ -12536,7 +12540,9 @@ grokdeclarator (const cp_declarator *declarator, unqualified_id); constexpr_p = false; } - decl = build_decl (input_location, + decl = build_decl (declarator + ? declarator->id_loc + : input_location, FIELD_DECL, unqualified_id, type); DECL_NONADDRESSABLE_P (decl) = bitfield; if (bitfield && !unqualified_id) Index: testsuite/g++.dg/other/bitfield2.C =================================================================== --- testsuite/g++.dg/other/bitfield2.C (revision 266840) +++ testsuite/g++.dg/other/bitfield2.C (working copy) @@ -3,7 +3,7 @@ struct A { - double d : 2; // { dg-error "non-integral" } + double d : 2; // { dg-error "10:bit-field .d. with non-integral type .double." } A() {} ~A() {} }; Index: testsuite/g++.dg/parse/bitfield1.C =================================================================== --- testsuite/g++.dg/parse/bitfield1.C (revision 266840) +++ testsuite/g++.dg/parse/bitfield1.C (working copy) @@ -2,7 +2,7 @@ struct A { - double i : 8; // { dg-error "type" } + double i : 8; // { dg-error "10:bit-field .i. with non-integral type .double." } }; void foo(A& a) Index: testsuite/g++.dg/parse/bitfield2.C =================================================================== --- testsuite/g++.dg/parse/bitfield2.C (revision 266840) +++ testsuite/g++.dg/parse/bitfield2.C (working copy) @@ -4,7 +4,7 @@ struct X {}; struct A { - X x : 2; // { dg-error "non-integral type" } + X x : 2; // { dg-error "7:bit-field .x. with non-integral type .X." } }; struct B : A {}; @@ -19,7 +19,7 @@ C<int> c; template <typename T> struct D { - T t : 3; // { dg-error "non-integral type" } + T t : 3; // { dg-error "5:bit-field .double D\\<double\\>::t. with non-integral type .double." } }; D<double> d; // { dg-message "required" } @@ -28,7 +28,7 @@ template <typename T> struct E { typedef T* U; - U t : 3; // { dg-error "non-integral type" } + U t : 3; // { dg-error "5:bit-field .t. with non-integral type .E\\<T\\>::U." } }; E<double> e; Index: testsuite/g++.dg/parse/bitfield7.C =================================================================== --- testsuite/g++.dg/parse/bitfield7.C (nonexistent) +++ testsuite/g++.dg/parse/bitfield7.C (working copy) @@ -0,0 +1,4 @@ +struct A +{ + friend int : 1; // { dg-error "unnamed field" } +};