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

[Paolo Carlini <paolo.carlini@oracle.com>]

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