[C++ Patch] Three additional bitfield diagnostic tweaks (a regression fix included)

[C++ Patch] Three additional bitfield diagnostic tweaks (a regression fix included)

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]

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.

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.

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.

The added/extended testcase exercise all of the above. Tested x86_64-linux.

Thanks, Paolo.

////////////////


[-- Attachment #2: CL_grokbitfield2 --]
[-- Type: text/plain, Size: 555 bytes --]

/cp
2018-12-06  Paolo Carlini  <paolo.carlini@oracle.com>

	* class.c (check_bitfield_decl): In error message about non-integral
	type print the type itself too.
	* decl.c (grokdeclarator): Do not ICE on unnamed bit-fields declared
	friends; when calling build_decl for a FIELD_DECL possibly pass the
	declarator->id_loc.

/testsuite
2018-12-06  Paolo Carlini  <paolo.carlini@oracle.com>

	* g++.dg/parse/bitfield7.C: New.
	* g++.dg/other/bitfield2.C: Check location and type.
	* g++.dg/parse/bitfield1.C: Likewise.
	* g++.dg/parse/bitfield2.C: Likewise.

[-- Attachment #3: patch_grokbitfield2 --]
[-- Type: text/plain, Size: 3739 bytes --]

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" }
+};

Re: [C++ Patch] Three additional bitfield diagnostic tweaks (a regression fix included)

[Jason Merrill]

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

Re: [C++ Patch] Three additional bitfield diagnostic tweaks (a regression fix included)

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 2992 bytes --]

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.


[-- Attachment #2: patch_grokbitfield2_2 --]
[-- Type: text/plain, Size: 4362 bytes --]

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" }
+};

Re: [C++ Patch] Three additional bitfield diagnostic tweaks (a regression fix included)

[Jason Merrill]

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