[C++] More location fixes to grokdeclarator

Message ID 67f2fc3d-020c-96b7-3176-2c8a8687c758@oracle.com
State New
Headers show
Series
  • [C++] More location fixes to grokdeclarator
Related show

Commit Message

Paolo Carlini June 25, 2018, 11:44 p.m.
Hi,

this includes straightforward tweaks to check_concept_fn and quite a bit 
of additional work on grokdeclarator: most of it is also rather 
straightforward. In a few places there is the subtlety that we want to 
handle together ds_storage_class and ds_thread, whichever location is 
the smallest but != UNKNOWN_LOCATION (UNKNWON_LOCATION meaning that the 
issue is with the other one) or use the biggest location when say 
ds_virtual and ds_storage_class conflict, because - I believe - we want 
to point to the place where we give up. Thus I added the min_location 
and max_location helpers. In one place - storage class specified for 
parameter - I also changed grokdeclarator to immediately return 
error_mark_node upon error, consistently with the other cases nearby, 
which avoids redundant diagnostic, that is two errors for a single 
issue. Tested x86_64-linux.

Thanks, Paolo.

PS: In Rapperswil we approved P1064R0, thus the limitation that a 
constexpr function can't be virtual will not exist in C++20 mode.

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

	* decl.c (min_location, max_location): New.
	(check_concept_fn): Use  DECL_SOURCE_LOCATION.
	(grokdeclarator): Use accurate locations in a number of error
	messages involving ds_thread, ds_storage_class, ds_virtual,
	ds_constexpr, ds_typedef and ds_friend; exploit min_location
	and max_location.

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

	* g++.dg/other/locations1.C: New.
	* g++.dg/tls/locations1.C: Likewise.
	* g++.dg/concepts/fn-concept2.C: Test the locations too.
	* g++.dg/cpp0x/constexpr-virtual5.C: Likewise.
	* g++.dg/cpp0x/pr51463.C: Likewise.
	* g++.dg/other/typedef1.C: Likewise.
	* g++.dg/parse/dtor13.C: Likewise.
	* g++.dg/template/error44.C: Likewise.
	* g++.dg/template/typedef4.C: Likewise.
	* g++.dg/template/typedef5.C: Likewise.
	* g++.dg/tls/diag-2.C: Likewise.
	* g++.old-deja/g++.brendan/crash11.C: Likewise.

Comments

David Malcolm June 26, 2018, 11:52 p.m. | #1
On Tue, 2018-06-26 at 01:44 +0200, Paolo Carlini wrote:
> Hi,

> 

> this includes straightforward tweaks to check_concept_fn and quite a

> bit 

> of additional work on grokdeclarator: most of it is also rather 

> straightforward. In a few places there is the subtlety that we want

> to 

> handle together ds_storage_class and ds_thread, whichever location

> is 

> the smallest but != UNKNOWN_LOCATION (UNKNWON_LOCATION meaning that

> the 

> issue is with the other one) or use the biggest location when say 

> ds_virtual and ds_storage_class conflict, because - I believe - we

> want 

> to point to the place where we give up. Thus I added the

> min_location 

> and max_location helpers. 


Note that directly comparing location_t values can be problematic: (one
value might be an ad-hoc location, and the other not; one might be a
macro expansion, etc).

You might want to use linemap_compare_locations or
linemap_location_before_p for this.

Hope this is helpful
Dave
Paolo Carlini June 27, 2018, 9:11 a.m. | #2
Hi David,

On 27/06/2018 01:52, David Malcolm wrote:
> On Tue, 2018-06-26 at 01:44 +0200, Paolo Carlini wrote:

>> Hi,

>>

>> this includes straightforward tweaks to check_concept_fn and quite a

>> bit

>> of additional work on grokdeclarator: most of it is also rather

>> straightforward. In a few places there is the subtlety that we want

>> to

>> handle together ds_storage_class and ds_thread, whichever location

>> is

>> the smallest but != UNKNOWN_LOCATION (UNKNWON_LOCATION meaning that

>> the

>> issue is with the other one) or use the biggest location when say

>> ds_virtual and ds_storage_class conflict, because - I believe - we

>> want

>> to point to the place where we give up. Thus I added the

>> min_location

>> and max_location helpers.

> Note that directly comparing location_t values can be problematic: (one

> value might be an ad-hoc location, and the other not; one might be a

> macro expansion, etc).

>

> You might want to use linemap_compare_locations or

> linemap_location_before_p for this.

Thanks David, I was not aware of this issue. In the below I amended the 
new functions and the existing smallest_type_quals_location (which I 
wrote a while ago) to use linemap_location_before_p.

Thanks again,
Paolo.

/////////////////////
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 262178)
+++ cp/decl.c	(working copy)
@@ -8545,15 +8545,18 @@ check_concept_fn (tree fn)
 {
   // A constraint is nullary.
   if (DECL_ARGUMENTS (fn))
-    error ("concept %q#D declared with function parameters", fn);
+    error_at (DECL_SOURCE_LOCATION (fn),
+	      "concept %q#D declared with function parameters", fn);
 
   // The declared return type of the concept shall be bool, and
   // it shall not be deduced from it definition.
   tree type = TREE_TYPE (TREE_TYPE (fn));
   if (is_auto (type))
-    error ("concept %q#D declared with a deduced return type", fn);
+    error_at (DECL_SOURCE_LOCATION (fn),
+	      "concept %q#D declared with a deduced return type", fn);
   else if (type != boolean_type_node)
-    error ("concept %q#D with non-%<bool%> return type %qT", fn, type);
+    error_at (DECL_SOURCE_LOCATION (fn),
+	      "concept %q#D with non-%<bool%> return type %qT", fn, type);
 }
 
 /* Helper function.  Replace the temporary this parameter injected
@@ -9808,16 +9811,42 @@ smallest_type_quals_location (int type_quals, cons
     loc = locations[ds_const];
 
   if ((type_quals & TYPE_QUAL_VOLATILE)
-      && (loc == UNKNOWN_LOCATION || locations[ds_volatile] < loc))
+      && (loc == UNKNOWN_LOCATION
+	  || linemap_location_before_p (line_table,
+					locations[ds_volatile], loc)))
     loc = locations[ds_volatile];
 
   if ((type_quals & TYPE_QUAL_RESTRICT)
-      && (loc == UNKNOWN_LOCATION || locations[ds_restrict] < loc))
+      && (loc == UNKNOWN_LOCATION
+	  || linemap_location_before_p (line_table,
+					locations[ds_restrict], loc)))
     loc = locations[ds_restrict];
 
   return loc;
 }
 
+/* Returns the smallest location.  */
+
+static location_t
+min_location (location_t loca, location_t locb)
+{
+  if (loca == UNKNOWN_LOCATION
+      || (locb != UNKNOWN_LOCATION
+	  && linemap_location_before_p (line_table, locb, loca)))
+    return locb;
+  return loca;
+}
+
+/* Returns the biggest location.  */
+
+static location_t
+max_location (location_t loca, location_t locb)
+{
+  if (linemap_location_before_p (line_table, loca, locb))
+    return locb;
+  return loca;
+}
+
 /* Check that it's OK to declare a function with the indicated TYPE
    and TYPE_QUALS.  SFK indicates the kind of special function (if any)
    that this function is.  OPTYPE is the type given in a conversion
@@ -10710,14 +10739,18 @@ grokdeclarator (const cp_declarator *declarator,
     {
       if (staticp == 2)
 	{
-	  error ("member %qD cannot be declared both %<virtual%> "
-		 "and %<static%>", dname);
+	  error_at (max_location (declspecs->locations[ds_virtual],
+				  declspecs->locations[ds_storage_class]),
+		    "member %qD cannot be declared both %<virtual%> "
+		    "and %<static%>", dname);
 	  storage_class = sc_none;
 	  staticp = 0;
 	}
       if (constexpr_p)
-	error ("member %qD cannot be declared both %<virtual%> "
-	       "and %<constexpr%>", dname);
+	error_at (max_location (declspecs->locations[ds_virtual],
+				declspecs->locations[ds_constexpr]),
+		  "member %qD cannot be declared both %<virtual%> "
+		  "and %<constexpr%>", dname);
     }
   friendp = decl_spec_seq_has_spec_p (declspecs, ds_friend);
 
@@ -10726,18 +10759,27 @@ grokdeclarator (const cp_declarator *declarator,
     {
       if (typedef_p)
 	{
-	  error ("typedef declaration invalid in parameter declaration");
+	  error_at (declspecs->locations[ds_typedef],
+		    "typedef declaration invalid in parameter declaration");
 	  return error_mark_node;
 	}
       else if (template_parm_flag && storage_class != sc_none)
 	{
-	  error ("storage class specified for template parameter %qs", name);
+	  error_at (min_location (declspecs->locations[ds_thread],
+				  declspecs->locations[ds_storage_class]),
+		    "storage class specified for template parameter %qs",
+		    name);
 	  return error_mark_node;
 	}
       else if (storage_class == sc_static
 	       || storage_class == sc_extern
 	       || thread_p)
-	error ("storage class specifiers invalid in parameter declarations");
+	{
+	  error_at (min_location (declspecs->locations[ds_thread],
+				  declspecs->locations[ds_storage_class]),
+		    "storage class specified for parameter %qs", name);
+	  return error_mark_node;
+	}
 
       /* Function parameters cannot be concept. */
       if (concept_p)
@@ -10872,13 +10914,19 @@ grokdeclarator (const cp_declarator *declarator,
       else
 	{
 	  if (decl_context == FIELD)
-	    error ("storage class specified for %qs", name);
+	    error_at (min_location (declspecs->locations[ds_thread],
+				    declspecs->locations[ds_storage_class]),
+		      "storage class specified for %qs", name);
 	  else
 	    {
 	      if (decl_context == PARM || decl_context == CATCHPARM)
-		error ("storage class specified for parameter %qs", name);
+		error_at (min_location (declspecs->locations[ds_thread],
+					declspecs->locations[ds_storage_class]),
+			  "storage class specified for parameter %qs", name);
 	      else
-		error ("storage class specified for typename");
+		error_at (min_location (declspecs->locations[ds_thread],
+					declspecs->locations[ds_storage_class]),
+			  "storage class specified for typename");
 	    }
 	  if (storage_class == sc_register
 	      || storage_class == sc_auto
@@ -10900,7 +10948,8 @@ grokdeclarator (const cp_declarator *declarator,
 	   && storage_class != sc_static)
     {
       if (declspecs->gnu_thread_keyword_p)
-	pedwarn (input_location, 0, "function-scope %qs implicitly auto and "
+	pedwarn (declspecs->locations[ds_thread],
+		 0, "function-scope %qs implicitly auto and "
 		 "declared %<__thread%>", name);
 
       /* When thread_local is applied to a variable of block scope the
@@ -10912,7 +10961,10 @@ grokdeclarator (const cp_declarator *declarator,
 
   if (storage_class && friendp)
     {
-      error ("storage class specifiers invalid in friend function declarations");
+      error_at (min_location (declspecs->locations[ds_thread],
+			      declspecs->locations[ds_storage_class]),
+		"storage class specifiers invalid in friend function "
+		"declarations");
       storage_class = sc_none;
       staticp = 0;
     }
@@ -11238,7 +11290,8 @@ grokdeclarator (const cp_declarator *declarator,
 		if (virtualp)
 		  {
 		    /* Cannot be both friend and virtual.  */
-		    error ("virtual functions cannot be friends");
+		    error_at (declspecs->locations[ds_friend],
+			      "virtual functions cannot be friends");
 		    friendp = 0;
 		  }
 		if (decl_context == NORMAL)
@@ -12369,15 +12422,18 @@ grokdeclarator (const cp_declarator *declarator,
 	else if (thread_p)
 	  {
 	    if (declspecs->gnu_thread_keyword_p)
-	      error ("storage class %<__thread%> invalid for function %qs",
-		     name);
+	      error_at (declspecs->locations[ds_thread],
+			"storage class %<__thread%> invalid for function %qs",
+			name);
 	    else
-	      error ("storage class %<thread_local%> invalid for function %qs",
-		     name);
+	      error_at (declspecs->locations[ds_thread],
+			"storage class %<thread_local%> invalid for "
+			"function %qs", name);
 	  }
 
         if (virt_specifiers)
-          error ("virt-specifiers in %qs not allowed outside a class definition", name);
+          error ("virt-specifiers in %qs not allowed outside a class "
+		 "definition", name);
 	/* Function declaration not at top level.
 	   Storage classes other than `extern' are not allowed
 	   and `extern' makes no difference.  */
Index: testsuite/g++.dg/concepts/fn-concept2.C
===================================================================
--- testsuite/g++.dg/concepts/fn-concept2.C	(revision 262178)
+++ testsuite/g++.dg/concepts/fn-concept2.C	(working copy)
@@ -1,7 +1,10 @@
 // { dg-options "-std=c++17 -fconcepts" }
 
 template<typename T>
-  concept auto C1() { return 0; } // { dg-error "deduced return type" }
+  concept auto C1() { return 0; } // { dg-error "16:concept .concept auto C1\\(\\). declared with a deduced return type" }
 
 template<typename T>
-  concept int C2() { return 0; } // { dg-error "return type" }
+  concept int C2() { return 0; } // { dg-error "15:concept .concept int C2\\(\\). with non-.bool. return type .int." }
+
+template<typename T>
+  concept bool C3(int) { return 0; } // { dg-error "16:concept .concept bool C3\\(int\\). declared with function parameters" }
Index: testsuite/g++.dg/cpp0x/constexpr-virtual5.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-virtual5.C	(revision 262178)
+++ testsuite/g++.dg/cpp0x/constexpr-virtual5.C	(working copy)
@@ -2,5 +2,5 @@
 // { dg-do compile { target c++11 } }
 
 struct S {
-  constexpr virtual int f() { return 1; }  // { dg-error "both 'virtual' and 'constexpr'" }
+  constexpr virtual int f() { return 1; }  // { dg-error "13:member .f. cannot be declared both .virtual. and .constexpr." }
 };
Index: testsuite/g++.dg/cpp0x/pr51463.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr51463.C	(revision 262178)
+++ testsuite/g++.dg/cpp0x/pr51463.C	(working copy)
@@ -3,5 +3,6 @@
 
 struct A
 {
-  static virtual int i = 0;	// { dg-error "both 'virtual' and 'static'|declared as" }
+  static virtual int i = 0;	// { dg-error "10:member .i. cannot be declared both .virtual. and .static." }
+  // { dg-error "declared as" "" { target *-*-* } .-1 }
 };
Index: testsuite/g++.dg/other/locations1.C
===================================================================
--- testsuite/g++.dg/other/locations1.C	(nonexistent)
+++ testsuite/g++.dg/other/locations1.C	(working copy)
@@ -0,0 +1 @@
+void foo(static int p);  // { dg-error "10:storage class specified" }
Index: testsuite/g++.dg/other/typedef1.C
===================================================================
--- testsuite/g++.dg/other/typedef1.C	(revision 262178)
+++ testsuite/g++.dg/other/typedef1.C	(working copy)
@@ -1,7 +1,10 @@
 // PR c++/27572
 // { dg-do compile }
 
-void f1(typedef) {}        // { dg-error "no type|typedef declaration" }
-void f2(typedef x) {}      // { dg-error "type|typedef declaration" }
-void f3(typedef x[]) {}    // { dg-error "type|typedef declaration" }
-void f4(typedef int x) {}  // { dg-error "typedef declaration" }
+void f1(typedef) {}        // { dg-error "9:typedef declaration" }
+// { dg-error "no type" "" { target *-*-* } .-1 }
+void f2(typedef x) {}      // { dg-error "9:typedef declaration" }
+// { dg-error "type" "" { target *-*-* } .-1 }
+void f3(typedef x[]) {}    // { dg-error "9:typedef declaration" }
+// { dg-error "type" "" { target *-*-* } .-1 }
+void f4(typedef int x) {}  // { dg-error "9:typedef declaration" }
Index: testsuite/g++.dg/parse/dtor13.C
===================================================================
--- testsuite/g++.dg/parse/dtor13.C	(revision 262178)
+++ testsuite/g++.dg/parse/dtor13.C	(working copy)
@@ -3,6 +3,7 @@
 
 struct A
 {
-  static friend A::~A(); /* { dg-error "storage class specifiers|extra qualification|implicitly friend" } */
+  static friend A::~A(); /* { dg-error "3:storage class specifiers" } */
+  /* { dg-error "extra qualification|implicitly friend" "" { target *-*-* } .-1 } */
 };
 
Index: testsuite/g++.dg/template/error44.C
===================================================================
--- testsuite/g++.dg/template/error44.C	(revision 262178)
+++ testsuite/g++.dg/template/error44.C	(working copy)
@@ -1,7 +1,8 @@
 // PR c++/32056
 
-template <auto int T> struct A {}; // { dg-error "storage class specified|two or more" }
-template <extern int T> struct B {}; // { dg-error "storage class specified" }
-template <static int T> struct C {}; // { dg-error "storage class specified" }
-template <register int T> struct D {}; // { dg-error "storage class specified" }
-template <mutable int T> struct E {}; // { dg-error "storage class specified" }
+template <auto int T> struct A {}; // { dg-error "11:storage class specified" "" { target c++98_only } }
+// { dg-error "two or more" "" { target c++11 } .-1 }
+template <extern int T> struct B {}; // { dg-error "11:storage class specified" }
+template <static int T> struct C {}; // { dg-error "11:storage class specified" }
+template <register int T> struct D {}; // { dg-error "11:storage class specified" }
+template <mutable int T> struct E {}; // { dg-error "11:storage class specified" }
Index: testsuite/g++.dg/template/typedef4.C
===================================================================
--- testsuite/g++.dg/template/typedef4.C	(revision 262178)
+++ testsuite/g++.dg/template/typedef4.C	(working copy)
@@ -1,7 +1,8 @@
 // PR c++/27572
 // { dg-do compile }
 
-template<typedef> void foo();  // { dg-error "no type|typedef declaration|template" }
+template<typedef> void foo();  // { dg-error "10:typedef declaration" }
+// { dg-error "no type|template" "" { target *-*-* } .-1 }
 
 void bar()
 {
Index: testsuite/g++.dg/template/typedef5.C
===================================================================
--- testsuite/g++.dg/template/typedef5.C	(revision 262178)
+++ testsuite/g++.dg/template/typedef5.C	(working copy)
@@ -1,7 +1,10 @@
 // PR c++/27572
 // { dg-do compile }
 
-template<typedef,int>        struct A1; // { dg-error "no type|typedef declaration|default argument" }
-template<typedef x,int>      struct A2; // { dg-error "type|typedef declaration|default argument" }
-template<typedef x[],int>    struct A3; // { dg-error "no type|typedef declaration|expected" }
-template<typedef int x, int> struct A4; // { dg-error "typedef declaration|default argument" }
+template<typedef,int>        struct A1; // { dg-error "10:typedef declaration" }
+// { dg-error "no type|default argument" "" { target *-*-* } .-1 }
+template<typedef x,int>      struct A2; // { dg-error "10:typedef declaration" }
+// { dg-error "type|default argument" "" { target *-*-* } .-1 }
+template<typedef x[],int>    struct A3; // { dg-error "typedef declaration|no type|expected" }
+template<typedef int x, int> struct A4; // { dg-error "10:typedef declaration" }
+// { dg-error "default argument" "" { target *-*-* } .-1 }
Index: testsuite/g++.dg/tls/diag-2.C
===================================================================
--- testsuite/g++.dg/tls/diag-2.C	(revision 262178)
+++ testsuite/g++.dg/tls/diag-2.C	(working copy)
@@ -8,19 +8,19 @@ typedef __thread int g4;	/* { dg-error "multiple s
 
 void foo()
 {
-  __thread int l1;		/* { dg-error "implicitly auto and declared '__thread'" } */
+  __thread int l1;		/* { dg-error "3:function-scope .l1. implicitly auto and declared '__thread'" } */
   auto __thread int l2;		/* { dg-error "multiple storage classes|data types" } */
   __thread extern int l3;	/* { dg-error "'__thread' before 'extern'" } */
   register __thread int l4;	/* { dg-error "multiple storage classes" } */
 }				/* { dg-error "ISO C\\+\\+17 does not allow 'register' storage class specifier" "" { target c++17 } .-1 } */
 
-__thread void f1 ();		/* { dg-error "invalid for function" } */
-extern __thread void f2 ();	/* { dg-error "invalid for function" } */
-static __thread void f3 ();	/* { dg-error "invalid for function" } */
-__thread void f4 () { }		/* { dg-error "invalid for function" } */
+__thread void f1 ();		/* { dg-error "1:storage class .__thread. invalid for function" } */
+extern __thread void f2 ();	/* { dg-error "8:storage class .__thread. invalid for function" } */
+static __thread void f3 ();	/* { dg-error "8:storage class .__thread. invalid for function" } */
+__thread void f4 () { }		/* { dg-error "1:storage class .__thread. invalid for function" } */
 
-void bar(__thread int p1);	/* { dg-error "(invalid in parameter)|(specified for parameter)" } */
+void bar(__thread int p1);	/* { dg-error "10:storage class specified for parameter" } */
 
 struct A {
-  __thread int i;		/* { dg-error "storage class specified" } */
+  __thread int i;		/* { dg-error "3:storage class specified" } */
 };
Index: testsuite/g++.dg/tls/locations1.C
===================================================================
--- testsuite/g++.dg/tls/locations1.C	(nonexistent)
+++ testsuite/g++.dg/tls/locations1.C	(working copy)
@@ -0,0 +1,3 @@
+/* { dg-require-effective-target tls } */
+
+template <__thread int T> struct F {}; // { dg-error "11:storage class specified" }
Index: testsuite/g++.old-deja/g++.brendan/crash11.C
===================================================================
--- testsuite/g++.old-deja/g++.brendan/crash11.C	(revision 262178)
+++ testsuite/g++.old-deja/g++.brendan/crash11.C	(working copy)
@@ -9,13 +9,14 @@ class A {
 	int	h;
 	A() { i=10; j=20; }
 	virtual void f1() { printf("i=%d j=%d\n",i,j); }
-	friend virtual void f2() { printf("i=%d j=%d\n",i,j); }// { dg-error "" }  virtual.*
+	friend virtual void f2() { printf("i=%d j=%d\n",i,j); } // { dg-error "2:virtual functions cannot be friends" }
 };
 
 class B : public A {
     public:
 	virtual void f1() { printf("i=%d j=%d\n",i,j); }// { dg-error "" }  member.*// ERROR -  member.*
-	friend virtual void f2() { printf("i=%d j=%d\n",i,j); }// { dg-error "" }  virtual.*// ERROR -  member.*// ERROR -  member.*
+	friend virtual void f2() { printf("i=%d j=%d\n",i,j); }  // { dg-error "2:virtual functions cannot be friends" }
+// { dg-error "private" "" { target *-*-* } .-1 }
 };
 
 int
Jason Merrill June 27, 2018, 11:31 p.m. | #3
On Wed, Jun 27, 2018 at 5:11 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi David,

>

> On 27/06/2018 01:52, David Malcolm wrote:

>>

>> On Tue, 2018-06-26 at 01:44 +0200, Paolo Carlini wrote:

>>>

>>> Hi,

>>>

>>> this includes straightforward tweaks to check_concept_fn and quite a

>>> bit

>>> of additional work on grokdeclarator: most of it is also rather

>>> straightforward. In a few places there is the subtlety that we want

>>> to

>>> handle together ds_storage_class and ds_thread, whichever location

>>> is

>>> the smallest but != UNKNOWN_LOCATION (UNKNWON_LOCATION meaning that

>>> the

>>> issue is with the other one) or use the biggest location when say

>>> ds_virtual and ds_storage_class conflict, because - I believe - we

>>> want

>>> to point to the place where we give up. Thus I added the

>>> min_location

>>> and max_location helpers.

>>

>> Note that directly comparing location_t values can be problematic: (one

>> value might be an ad-hoc location, and the other not; one might be a

>> macro expansion, etc).

>>

>> You might want to use linemap_compare_locations or

>> linemap_location_before_p for this.

>

> Thanks David, I was not aware of this issue. In the below I amended the new

> functions and the existing smallest_type_quals_location (which I wrote a

> while ago) to use linemap_location_before_p.


> +/* Returns the smallest location.  */


This should probably say "...that is not UNKNOWN_LOCATION."

Actually, the places you use min_location would seem to work fine with
max_location as well.  What are your criteria for choosing one or the
other?

Jason
Paolo Carlini June 28, 2018, 12:28 a.m. | #4
Hi,

On 28/06/2018 01:31, Jason Merrill wrote:
>

>> +/* Returns the smallest location.  */

> This should probably say "...that is not UNKNOWN_LOCATION."

I agree.
> Actually, the places you use min_location would seem to work fine with

> max_location as well.  What are your criteria for choosing one or the

> other?

I should have explained that in better detail. I see two different 
circumstances: either we have error messages where we say something like 
"cannot be both":

-	  error ("member %qD cannot be declared both %<virtual%> "
-		 "and %<static%>", dname);
+	  error_at (max_location (declspecs->locations[ds_virtual],
+				  declspecs->locations[ds_storage_class]),
+		    "member %qD cannot be declared both %<virtual%> "
+		    "and %<static%>", dname);

where, in my opinion, we want to point to the max_location, we want to point to where the contradiction shows up in the code. Or, we have errors like:

-	  error ("storage class specified for template parameter %qs", name);
+	  error_at (min_location (declspecs->locations[ds_thread],
+				  declspecs->locations[ds_storage_class]),
+		    "storage class specified for template parameter %qs",
+		    name);

where ill-formed code has either one or two such specifiers (that is __thread and/or static) but even one would wrong, thus we want to point to the first one, thus min_location (this is in fact a variant of the reasoning behind smallest_type_quals_location).

Did I explain myself clearly enough? If we are going for something simple, I would suggest uniformly using min_location, not max_location.

By the way, I think I found examples of locations following both the patterns above in clang and icc too.

Thanks,
Paolo.
David Malcolm June 28, 2018, 1:13 a.m. | #5
On Thu, 2018-06-28 at 02:28 +0200, Paolo Carlini wrote:
> Hi,

> 

> On 28/06/2018 01:31, Jason Merrill wrote:

> > 

> > > +/* Returns the smallest location.  */

> > 

> > This should probably say "...that is not UNKNOWN_LOCATION."

> 

> I agree.

> > Actually, the places you use min_location would seem to work fine

> > with

> > max_location as well.  What are your criteria for choosing one or

> > the

> > other?

> 

> I should have explained that in better detail. I see two different 

> circumstances: either we have error messages where we say something

> like 

> "cannot be both":

> 

> -	  error ("member %qD cannot be declared both %<virtual%> "

> -		 "and %<static%>", dname);

> +	  error_at (max_location (declspecs->locations[ds_virtual],

> +				  declspecs-

> >locations[ds_storage_class]),

> +		    "member %qD cannot be declared both %<virtual%>

> "

> +		    "and %<static%>", dname);

> 

> where, in my opinion, we want to point to the max_location, we want

> to point to where the contradiction shows up in the code. Or, we have

> errors like:

> 

> -	  error ("storage class specified for template parameter

> %qs", name);

> +	  error_at (min_location (declspecs->locations[ds_thread],

> +				  declspecs-

> >locations[ds_storage_class]),

> +		    "storage class specified for template parameter

> %qs",

> +		    name);

> 

> where ill-formed code has either one or two such specifiers (that is

> __thread and/or static) but even one would wrong, thus we want to

> point to the first one, thus min_location (this is in fact a variant

> of the reasoning behind smallest_type_quals_location).

> 

> Did I explain myself clearly enough? If we are going for something

> simple, I would suggest uniformly using min_location, not

> max_location.


If I'm following you right, the idea is that gcc should complain
because two different things in the user's source code contradict each
other.

In such circumstances, I think we ought to try to print *both*
locations, so that we're showing, rather than just telling.

So the user experience might be something like:

test.cc:42:1: error: member 'bar::foo' cannot be declared both 'virtual' and 'static'
 static virtual foo ();
 ^~~~~~ ~~~~~~~

It's possible to send two locations through one diagnostic using
rich_location, via e.g.:

   gcc_rich_location richloc (primary_location);
   richloc.add_range (secondary_location, false);
   error_at (&richloc, "member %qD etc etc"", .../* etc*/);

For this case, I don't think it matters which one is primary and which
is secondary (it merely determines the file:line:column in the
preliminary message of the diagnostic).

Another approach would be to use two diagnostics, for something like:

test.cc:42:8: error: member 'foo' cannot be declared both 'virtual'...
 static virtual foo ();
        ^~~~~~~
test.cc:42:1: note: ...and 'static'
 static virtual foo ();
 ^~~~~~
 
but given that in this case the two things are likely to be close
together in the user's source, I prefer the "two locations in one
diagnostic" approach.

Hope this is constructive
Dave

> By the way, I think I found examples of locations following both the

> patterns above in clang and icc too.

> 

> Thanks,

> Paolo.

>
David Malcolm June 28, 2018, 1:22 a.m. | #6
On Wed, 2018-06-27 at 21:13 -0400, David Malcolm wrote:
> On Thu, 2018-06-28 at 02:28 +0200, Paolo Carlini wrote:

> > Hi,

> > 

> > On 28/06/2018 01:31, Jason Merrill wrote:

> > > 

> > > > +/* Returns the smallest location.  */

> > > 

> > > This should probably say "...that is not UNKNOWN_LOCATION."

> > 

> > I agree.

> > > Actually, the places you use min_location would seem to work fine

> > > with

> > > max_location as well.  What are your criteria for choosing one or

> > > the

> > > other?

> > 

> > I should have explained that in better detail. I see two different 

> > circumstances: either we have error messages where we say something

> > like 

> > "cannot be both":

> > 

> > -	  error ("member %qD cannot be declared both %<virtual%> "

> > -		 "and %<static%>", dname);

> > +	  error_at (max_location (declspecs-

> > >locations[ds_virtual],

> > +				  declspecs-

> > > locations[ds_storage_class]),

> > 

> > +		    "member %qD cannot be declared both

> > %<virtual%>

> > "

> > +		    "and %<static%>", dname);

> > 

> > where, in my opinion, we want to point to the max_location, we want

> > to point to where the contradiction shows up in the code. Or, we

> > have

> > errors like:

> > 

> > -	  error ("storage class specified for template parameter

> > %qs", name);

> > +	  error_at (min_location (declspecs->locations[ds_thread],

> > +				  declspecs-

> > > locations[ds_storage_class]),

> > 

> > +		    "storage class specified for template

> > parameter

> > %qs",

> > +		    name);

> > 

> > where ill-formed code has either one or two such specifiers (that

> > is

> > __thread and/or static) but even one would wrong, thus we want to

> > point to the first one, thus min_location (this is in fact a

> > variant

> > of the reasoning behind smallest_type_quals_location).

> > 

> > Did I explain myself clearly enough? If we are going for something

> > simple, I would suggest uniformly using min_location, not

> > max_location.

> 

> If I'm following you right, the idea is that gcc should complain

> because two different things in the user's source code contradict

> each

> other.

> 

> In such circumstances, I think we ought to try to print *both*

> locations, so that we're showing, rather than just telling.


Or to put in another way, if two things in the user's source contradict
each other, we should show the user both.  The user is going to have to
decide to delete one (or both) of them, and we don't know which one,
but at least by showing both it helps him/her take their next action.

> So the user experience might be something like:

> 

> test.cc:42:1: error: member 'bar::foo' cannot be declared both

> 'virtual' and 'static'

>  static virtual foo ();

>  ^~~~~~ ~~~~~~~

> 

> It's possible to send two locations through one diagnostic using

> rich_location, via e.g.:

> 

>    gcc_rich_location richloc (primary_location);

>    richloc.add_range (secondary_location, false);

>    error_at (&richloc, "member %qD etc etc"", .../* etc*/);

> 

> For this case, I don't think it matters which one is primary and

> which

> is secondary (it merely determines the file:line:column in the

> preliminary message of the diagnostic).

> 

> Another approach would be to use two diagnostics, for something like:

> 

> test.cc:42:8: error: member 'foo' cannot be declared both

> 'virtual'...

>  static virtual foo ();

>         ^~~~~~~

> test.cc:42:1: note: ...and 'static'

>  static virtual foo ();

>  ^~~~~~

>  

> but given that in this case the two things are likely to be close

> together in the user's source, I prefer the "two locations in one

> diagnostic" approach.

> 

> Hope this is constructive

> Dave

> 

> > By the way, I think I found examples of locations following both

> > the

> > patterns above in clang and icc too.

> > 

> > Thanks,

> > Paolo.

> >
Paolo Carlini June 28, 2018, 9:39 a.m. | #7
Hi,

On 28/06/2018 03:22, David Malcolm wrote:
> [snip]

>> If I'm following you right, the idea is that gcc should complain

>> because two different things in the user's source code contradict

>> each

>> other.

>>

>> In such circumstances, I think we ought to try to print *both*

>> locations, so that we're showing, rather than just telling.

> Or to put in another way, if two things in the user's source contradict

> each other, we should show the user both.  The user is going to have to

> decide to delete one (or both) of them, and we don't know which one,

> but at least by showing both it helps him/her take their next action.

Sure, makes sense. Thus the below uses rich_location the way you 
explained. I also added 2 specific testcases and extended a bit another 
one to exercise a bit more min_location..Of course the patch doesn't add 
max_location anymore, I suspect we are not going to find uses for a max 
anytime soon, because we really want rich_location with multiple ranges 
in all those cases...

Thanks!
Paolo.

/////////////////////////
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 262214)
+++ cp/decl.c	(working copy)
@@ -8545,15 +8545,18 @@ check_concept_fn (tree fn)
 {
   // A constraint is nullary.
   if (DECL_ARGUMENTS (fn))
-    error ("concept %q#D declared with function parameters", fn);
+    error_at (DECL_SOURCE_LOCATION (fn),
+	      "concept %q#D declared with function parameters", fn);
 
   // The declared return type of the concept shall be bool, and
   // it shall not be deduced from it definition.
   tree type = TREE_TYPE (TREE_TYPE (fn));
   if (is_auto (type))
-    error ("concept %q#D declared with a deduced return type", fn);
+    error_at (DECL_SOURCE_LOCATION (fn),
+	      "concept %q#D declared with a deduced return type", fn);
   else if (type != boolean_type_node)
-    error ("concept %q#D with non-%<bool%> return type %qT", fn, type);
+    error_at (DECL_SOURCE_LOCATION (fn),
+	      "concept %q#D with non-%<bool%> return type %qT", fn, type);
 }
 
 /* Helper function.  Replace the temporary this parameter injected
@@ -9808,16 +9811,32 @@ smallest_type_quals_location (int type_quals, cons
     loc = locations[ds_const];
 
   if ((type_quals & TYPE_QUAL_VOLATILE)
-      && (loc == UNKNOWN_LOCATION || locations[ds_volatile] < loc))
+      && (loc == UNKNOWN_LOCATION
+	  || linemap_location_before_p (line_table,
+					locations[ds_volatile], loc)))
     loc = locations[ds_volatile];
 
   if ((type_quals & TYPE_QUAL_RESTRICT)
-      && (loc == UNKNOWN_LOCATION || locations[ds_restrict] < loc))
+      && (loc == UNKNOWN_LOCATION
+	  || linemap_location_before_p (line_table,
+					locations[ds_restrict], loc)))
     loc = locations[ds_restrict];
 
   return loc;
 }
 
+/* Returns the smallest location that is not UNKNOWN_LOCATION.  */
+
+static location_t
+min_location (location_t loca, location_t locb)
+{
+  if (loca == UNKNOWN_LOCATION
+      || (locb != UNKNOWN_LOCATION
+	  && linemap_location_before_p (line_table, locb, loca)))
+    return locb;
+  return loca;
+}
+
 /* Check that it's OK to declare a function with the indicated TYPE
    and TYPE_QUALS.  SFK indicates the kind of special function (if any)
    that this function is.  OPTYPE is the type given in a conversion
@@ -10710,14 +10729,20 @@ grokdeclarator (const cp_declarator *declarator,
     {
       if (staticp == 2)
 	{
-	  error ("member %qD cannot be declared both %<virtual%> "
-		 "and %<static%>", dname);
+	  rich_location richloc (line_table, declspecs->locations[ds_virtual]);
+	  richloc.add_range (declspecs->locations[ds_storage_class], false);
+	  error_at (&richloc, "member %qD cannot be declared both %<virtual%> "
+		    "and %<static%>", dname);
 	  storage_class = sc_none;
 	  staticp = 0;
 	}
       if (constexpr_p)
-	error ("member %qD cannot be declared both %<virtual%> "
-	       "and %<constexpr%>", dname);
+	{
+	  rich_location richloc (line_table, declspecs->locations[ds_virtual]);
+	  richloc.add_range (declspecs->locations[ds_constexpr], false);
+	  error_at (&richloc, "member %qD cannot be declared both %<virtual%> "
+		    "and %<constexpr%>", dname);
+	}
     }
   friendp = decl_spec_seq_has_spec_p (declspecs, ds_friend);
 
@@ -10726,18 +10751,27 @@ grokdeclarator (const cp_declarator *declarator,
     {
       if (typedef_p)
 	{
-	  error ("typedef declaration invalid in parameter declaration");
+	  error_at (declspecs->locations[ds_typedef],
+		    "typedef declaration invalid in parameter declaration");
 	  return error_mark_node;
 	}
       else if (template_parm_flag && storage_class != sc_none)
 	{
-	  error ("storage class specified for template parameter %qs", name);
+	  error_at (min_location (declspecs->locations[ds_thread],
+				  declspecs->locations[ds_storage_class]),
+		    "storage class specified for template parameter %qs",
+		    name);
 	  return error_mark_node;
 	}
       else if (storage_class == sc_static
 	       || storage_class == sc_extern
 	       || thread_p)
-	error ("storage class specifiers invalid in parameter declarations");
+	{
+	  error_at (min_location (declspecs->locations[ds_thread],
+				  declspecs->locations[ds_storage_class]),
+		    "storage class specified for parameter %qs", name);
+	  return error_mark_node;
+	}
 
       /* Function parameters cannot be concept. */
       if (concept_p)
@@ -10872,13 +10906,19 @@ grokdeclarator (const cp_declarator *declarator,
       else
 	{
 	  if (decl_context == FIELD)
-	    error ("storage class specified for %qs", name);
+	    error_at (min_location (declspecs->locations[ds_thread],
+				    declspecs->locations[ds_storage_class]),
+		      "storage class specified for %qs", name);
 	  else
 	    {
 	      if (decl_context == PARM || decl_context == CATCHPARM)
-		error ("storage class specified for parameter %qs", name);
+		error_at (min_location (declspecs->locations[ds_thread],
+					declspecs->locations[ds_storage_class]),
+			  "storage class specified for parameter %qs", name);
 	      else
-		error ("storage class specified for typename");
+		error_at (min_location (declspecs->locations[ds_thread],
+					declspecs->locations[ds_storage_class]),
+			  "storage class specified for typename");
 	    }
 	  if (storage_class == sc_register
 	      || storage_class == sc_auto
@@ -10900,7 +10940,8 @@ grokdeclarator (const cp_declarator *declarator,
 	   && storage_class != sc_static)
     {
       if (declspecs->gnu_thread_keyword_p)
-	pedwarn (input_location, 0, "function-scope %qs implicitly auto and "
+	pedwarn (declspecs->locations[ds_thread],
+		 0, "function-scope %qs implicitly auto and "
 		 "declared %<__thread%>", name);
 
       /* When thread_local is applied to a variable of block scope the
@@ -10912,7 +10953,10 @@ grokdeclarator (const cp_declarator *declarator,
 
   if (storage_class && friendp)
     {
-      error ("storage class specifiers invalid in friend function declarations");
+      error_at (min_location (declspecs->locations[ds_thread],
+			      declspecs->locations[ds_storage_class]),
+		"storage class specifiers invalid in friend function "
+		"declarations");
       storage_class = sc_none;
       staticp = 0;
     }
@@ -11238,7 +11282,8 @@ grokdeclarator (const cp_declarator *declarator,
 		if (virtualp)
 		  {
 		    /* Cannot be both friend and virtual.  */
-		    error ("virtual functions cannot be friends");
+		    error_at (declspecs->locations[ds_friend],
+			      "virtual functions cannot be friends");
 		    friendp = 0;
 		  }
 		if (decl_context == NORMAL)
@@ -12369,15 +12414,18 @@ grokdeclarator (const cp_declarator *declarator,
 	else if (thread_p)
 	  {
 	    if (declspecs->gnu_thread_keyword_p)
-	      error ("storage class %<__thread%> invalid for function %qs",
-		     name);
+	      error_at (declspecs->locations[ds_thread],
+			"storage class %<__thread%> invalid for function %qs",
+			name);
 	    else
-	      error ("storage class %<thread_local%> invalid for function %qs",
-		     name);
+	      error_at (declspecs->locations[ds_thread],
+			"storage class %<thread_local%> invalid for "
+			"function %qs", name);
 	  }
 
         if (virt_specifiers)
-          error ("virt-specifiers in %qs not allowed outside a class definition", name);
+          error ("virt-specifiers in %qs not allowed outside a class "
+		 "definition", name);
 	/* Function declaration not at top level.
 	   Storage classes other than `extern' are not allowed
 	   and `extern' makes no difference.  */
Index: testsuite/g++.dg/concepts/fn-concept2.C
===================================================================
--- testsuite/g++.dg/concepts/fn-concept2.C	(revision 262214)
+++ testsuite/g++.dg/concepts/fn-concept2.C	(working copy)
@@ -1,7 +1,10 @@
 // { dg-options "-std=c++17 -fconcepts" }
 
 template<typename T>
-  concept auto C1() { return 0; } // { dg-error "deduced return type" }
+  concept auto C1() { return 0; } // { dg-error "16:concept .concept auto C1\\(\\). declared with a deduced return type" }
 
 template<typename T>
-  concept int C2() { return 0; } // { dg-error "return type" }
+  concept int C2() { return 0; } // { dg-error "15:concept .concept int C2\\(\\). with non-.bool. return type .int." }
+
+template<typename T>
+  concept bool C3(int) { return 0; } // { dg-error "16:concept .concept bool C3\\(int\\). declared with function parameters" }
Index: testsuite/g++.dg/cpp0x/constexpr-virtual5.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-virtual5.C	(revision 262214)
+++ testsuite/g++.dg/cpp0x/constexpr-virtual5.C	(working copy)
@@ -2,5 +2,5 @@
 // { dg-do compile { target c++11 } }
 
 struct S {
-  constexpr virtual int f() { return 1; }  // { dg-error "both 'virtual' and 'constexpr'" }
+  constexpr virtual int f() { return 1; }  // { dg-error "13:member .f. cannot be declared both .virtual. and .constexpr." }
 };
Index: testsuite/g++.dg/cpp0x/pr51463.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr51463.C	(revision 262214)
+++ testsuite/g++.dg/cpp0x/pr51463.C	(working copy)
@@ -3,5 +3,6 @@
 
 struct A
 {
-  static virtual int i = 0;	// { dg-error "both 'virtual' and 'static'|declared as" }
+  static virtual int i = 0;	// { dg-error "10:member .i. cannot be declared both .virtual. and .static." }
+  // { dg-error "declared as" "" { target *-*-* } .-1 }
 };
Index: testsuite/g++.dg/diagnostic/virtual-constexpr.C
===================================================================
--- testsuite/g++.dg/diagnostic/virtual-constexpr.C	(nonexistent)
+++ testsuite/g++.dg/diagnostic/virtual-constexpr.C	(working copy)
@@ -0,0 +1,16 @@
+// { dg-options "-fdiagnostics-show-caret" }
+// { dg-do compile { target c++11 } }
+
+struct S
+{
+  virtual constexpr void foo();  // { dg-error "3:member .foo. cannot be declared both .virtual. and .constexpr." }
+/* { dg-begin-multiline-output "" }
+   virtual constexpr void foo();
+   ^~~~~~~ ~~~~~~~~~
+   { dg-end-multiline-output "" } */
+  constexpr virtual void bar();  // { dg-error "13:member .bar. cannot be declared both .virtual. and .constexpr." }
+/* { dg-begin-multiline-output "" }
+   constexpr virtual void bar();
+   ~~~~~~~~~ ^~~~~~~
+   { dg-end-multiline-output "" } */
+};
Index: testsuite/g++.dg/diagnostic/virtual-static.C
===================================================================
--- testsuite/g++.dg/diagnostic/virtual-static.C	(nonexistent)
+++ testsuite/g++.dg/diagnostic/virtual-static.C	(working copy)
@@ -0,0 +1,15 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+struct S
+{
+  virtual static void foo();  // { dg-error "3:member .foo. cannot be declared both .virtual. and .static." }
+/* { dg-begin-multiline-output "" }
+   virtual static void foo();
+   ^~~~~~~ ~~~~~~
+   { dg-end-multiline-output "" } */
+  static virtual void bar();  // { dg-error "10:member .bar. cannot be declared both .virtual. and .static." }
+/* { dg-begin-multiline-output "" }
+   static virtual void bar();
+   ~~~~~~ ^~~~~~~
+   { dg-end-multiline-output "" } */
+};
Index: testsuite/g++.dg/other/locations1.C
===================================================================
--- testsuite/g++.dg/other/locations1.C	(nonexistent)
+++ testsuite/g++.dg/other/locations1.C	(working copy)
@@ -0,0 +1 @@
+void foo(static int p);  // { dg-error "10:storage class specified" }
Index: testsuite/g++.dg/other/typedef1.C
===================================================================
--- testsuite/g++.dg/other/typedef1.C	(revision 262214)
+++ testsuite/g++.dg/other/typedef1.C	(working copy)
@@ -1,7 +1,10 @@
 // PR c++/27572
 // { dg-do compile }
 
-void f1(typedef) {}        // { dg-error "no type|typedef declaration" }
-void f2(typedef x) {}      // { dg-error "type|typedef declaration" }
-void f3(typedef x[]) {}    // { dg-error "type|typedef declaration" }
-void f4(typedef int x) {}  // { dg-error "typedef declaration" }
+void f1(typedef) {}        // { dg-error "9:typedef declaration" }
+// { dg-error "no type" "" { target *-*-* } .-1 }
+void f2(typedef x) {}      // { dg-error "9:typedef declaration" }
+// { dg-error "type" "" { target *-*-* } .-1 }
+void f3(typedef x[]) {}    // { dg-error "9:typedef declaration" }
+// { dg-error "type" "" { target *-*-* } .-1 }
+void f4(typedef int x) {}  // { dg-error "9:typedef declaration" }
Index: testsuite/g++.dg/parse/dtor13.C
===================================================================
--- testsuite/g++.dg/parse/dtor13.C	(revision 262214)
+++ testsuite/g++.dg/parse/dtor13.C	(working copy)
@@ -3,6 +3,7 @@
 
 struct A
 {
-  static friend A::~A(); /* { dg-error "storage class specifiers|extra qualification|implicitly friend" } */
+  static friend A::~A(); /* { dg-error "3:storage class specifiers" } */
+  /* { dg-error "extra qualification|implicitly friend" "" { target *-*-* } .-1 } */
 };
 
Index: testsuite/g++.dg/template/error44.C
===================================================================
--- testsuite/g++.dg/template/error44.C	(revision 262214)
+++ testsuite/g++.dg/template/error44.C	(working copy)
@@ -1,7 +1,8 @@
 // PR c++/32056
 
-template <auto int T> struct A {}; // { dg-error "storage class specified|two or more" }
-template <extern int T> struct B {}; // { dg-error "storage class specified" }
-template <static int T> struct C {}; // { dg-error "storage class specified" }
-template <register int T> struct D {}; // { dg-error "storage class specified" }
-template <mutable int T> struct E {}; // { dg-error "storage class specified" }
+template <auto int T> struct A {}; // { dg-error "11:storage class specified" "" { target c++98_only } }
+// { dg-error "two or more" "" { target c++11 } .-1 }
+template <extern int T> struct B {}; // { dg-error "11:storage class specified" }
+template <static int T> struct C {}; // { dg-error "11:storage class specified" }
+template <register int T> struct D {}; // { dg-error "11:storage class specified" }
+template <mutable int T> struct E {}; // { dg-error "11:storage class specified" }
Index: testsuite/g++.dg/template/typedef4.C
===================================================================
--- testsuite/g++.dg/template/typedef4.C	(revision 262214)
+++ testsuite/g++.dg/template/typedef4.C	(working copy)
@@ -1,7 +1,8 @@
 // PR c++/27572
 // { dg-do compile }
 
-template<typedef> void foo();  // { dg-error "no type|typedef declaration|template" }
+template<typedef> void foo();  // { dg-error "10:typedef declaration" }
+// { dg-error "no type|template" "" { target *-*-* } .-1 }
 
 void bar()
 {
Index: testsuite/g++.dg/template/typedef5.C
===================================================================
--- testsuite/g++.dg/template/typedef5.C	(revision 262214)
+++ testsuite/g++.dg/template/typedef5.C	(working copy)
@@ -1,7 +1,10 @@
 // PR c++/27572
 // { dg-do compile }
 
-template<typedef,int>        struct A1; // { dg-error "no type|typedef declaration|default argument" }
-template<typedef x,int>      struct A2; // { dg-error "type|typedef declaration|default argument" }
-template<typedef x[],int>    struct A3; // { dg-error "no type|typedef declaration|expected" }
-template<typedef int x, int> struct A4; // { dg-error "typedef declaration|default argument" }
+template<typedef,int>        struct A1; // { dg-error "10:typedef declaration" }
+// { dg-error "no type|default argument" "" { target *-*-* } .-1 }
+template<typedef x,int>      struct A2; // { dg-error "10:typedef declaration" }
+// { dg-error "type|default argument" "" { target *-*-* } .-1 }
+template<typedef x[],int>    struct A3; // { dg-error "typedef declaration|no type|expected" }
+template<typedef int x, int> struct A4; // { dg-error "10:typedef declaration" }
+// { dg-error "default argument" "" { target *-*-* } .-1 }
Index: testsuite/g++.dg/tls/diag-2.C
===================================================================
--- testsuite/g++.dg/tls/diag-2.C	(revision 262214)
+++ testsuite/g++.dg/tls/diag-2.C	(working copy)
@@ -8,19 +8,19 @@ typedef __thread int g4;	/* { dg-error "multiple s
 
 void foo()
 {
-  __thread int l1;		/* { dg-error "implicitly auto and declared '__thread'" } */
+  __thread int l1;		/* { dg-error "3:function-scope .l1. implicitly auto and declared '__thread'" } */
   auto __thread int l2;		/* { dg-error "multiple storage classes|data types" } */
   __thread extern int l3;	/* { dg-error "'__thread' before 'extern'" } */
   register __thread int l4;	/* { dg-error "multiple storage classes" } */
 }				/* { dg-error "ISO C\\+\\+17 does not allow 'register' storage class specifier" "" { target c++17 } .-1 } */
 
-__thread void f1 ();		/* { dg-error "invalid for function" } */
-extern __thread void f2 ();	/* { dg-error "invalid for function" } */
-static __thread void f3 ();	/* { dg-error "invalid for function" } */
-__thread void f4 () { }		/* { dg-error "invalid for function" } */
+__thread void f1 ();		/* { dg-error "1:storage class .__thread. invalid for function" } */
+extern __thread void f2 ();	/* { dg-error "8:storage class .__thread. invalid for function" } */
+static __thread void f3 ();	/* { dg-error "8:storage class .__thread. invalid for function" } */
+__thread void f4 () { }		/* { dg-error "1:storage class .__thread. invalid for function" } */
 
-void bar(__thread int p1);	/* { dg-error "(invalid in parameter)|(specified for parameter)" } */
+void bar(__thread int p1);	/* { dg-error "10:storage class specified for parameter" } */
 
 struct A {
-  __thread int i;		/* { dg-error "storage class specified" } */
+  __thread int i;		/* { dg-error "3:storage class specified" } */
 };
Index: testsuite/g++.dg/tls/locations1.C
===================================================================
--- testsuite/g++.dg/tls/locations1.C	(nonexistent)
+++ testsuite/g++.dg/tls/locations1.C	(working copy)
@@ -0,0 +1,4 @@
+/* { dg-require-effective-target tls } */
+
+template <__thread int T> struct F {}; // { dg-error "11:storage class specified" }
+template <static __thread int T> struct G {}; // { dg-error "11:storage class specified" }
Index: testsuite/g++.old-deja/g++.brendan/crash11.C
===================================================================
--- testsuite/g++.old-deja/g++.brendan/crash11.C	(revision 262214)
+++ testsuite/g++.old-deja/g++.brendan/crash11.C	(working copy)
@@ -9,13 +9,14 @@ class A {
 	int	h;
 	A() { i=10; j=20; }
 	virtual void f1() { printf("i=%d j=%d\n",i,j); }
-	friend virtual void f2() { printf("i=%d j=%d\n",i,j); }// { dg-error "" }  virtual.*
+	friend virtual void f2() { printf("i=%d j=%d\n",i,j); } // { dg-error "2:virtual functions cannot be friends" }
 };
 
 class B : public A {
     public:
 	virtual void f1() { printf("i=%d j=%d\n",i,j); }// { dg-error "" }  member.*// ERROR -  member.*
-	friend virtual void f2() { printf("i=%d j=%d\n",i,j); }// { dg-error "" }  virtual.*// ERROR -  member.*// ERROR -  member.*
+	friend virtual void f2() { printf("i=%d j=%d\n",i,j); }  // { dg-error "2:virtual functions cannot be friends" }
+// { dg-error "private" "" { target *-*-* } .-1 }
 };
 
 int
Jason Merrill July 3, 2018, 4:36 p.m. | #8
On Thu, Jun 28, 2018 at 5:39 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,

>

> On 28/06/2018 03:22, David Malcolm wrote:

>>

>> [snip]

>>>

>>> If I'm following you right, the idea is that gcc should complain

>>> because two different things in the user's source code contradict

>>> each

>>> other.

>>>

>>> In such circumstances, I think we ought to try to print *both*

>>> locations, so that we're showing, rather than just telling.

>>

>> Or to put in another way, if two things in the user's source contradict

>> each other, we should show the user both.  The user is going to have to

>> decide to delete one (or both) of them, and we don't know which one,

>> but at least by showing both it helps him/her take their next action.

>

> Sure, makes sense. Thus the below uses rich_location the way you explained.

> I also added 2 specific testcases and extended a bit another one to exercise

> a bit more min_location..Of course the patch doesn't add max_location

> anymore, I suspect we are not going to find uses for a max anytime soon,

> because we really want rich_location with multiple ranges in all those

> cases...


>    if ((type_quals & TYPE_QUAL_VOLATILE)

> -      && (loc == UNKNOWN_LOCATION || locations[ds_volatile] < loc))

> +      && (loc == UNKNOWN_LOCATION

> +          || linemap_location_before_p (line_table,

> +                                        locations[ds_volatile], loc)))

>      loc = locations[ds_volatile];


>    if ((type_quals & TYPE_QUAL_RESTRICT)

> -      && (loc == UNKNOWN_LOCATION || locations[ds_restrict] < loc))

> +      && (loc == UNKNOWN_LOCATION

> +          || linemap_location_before_p (line_table,

> +                                        locations[ds_restrict], loc)))

>      loc = locations[ds_restrict];


Why not use min_location here?

Jason
Paolo Carlini July 3, 2018, 6:37 p.m. | #9
Hi,

On 03/07/2018 18:36, Jason Merrill wrote:
>

>>     if ((type_quals & TYPE_QUAL_VOLATILE)

>> -      && (loc == UNKNOWN_LOCATION || locations[ds_volatile] < loc))

>> +      && (loc == UNKNOWN_LOCATION

>> +          || linemap_location_before_p (line_table,

>> +                                        locations[ds_volatile], loc)))

>>       loc = locations[ds_volatile];

>>     if ((type_quals & TYPE_QUAL_RESTRICT)

>> -      && (loc == UNKNOWN_LOCATION || locations[ds_restrict] < loc))

>> +      && (loc == UNKNOWN_LOCATION

>> +          || linemap_location_before_p (line_table,

>> +                                        locations[ds_restrict], loc)))

>>       loc = locations[ds_restrict];

> Why not use min_location here?

Indeed. Thus I successfully tested the below.

Thanks,
Paolo.

//////////////////////
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 262329)
+++ cp/decl.c	(working copy)
@@ -8545,15 +8545,18 @@ check_concept_fn (tree fn)
 {
   // A constraint is nullary.
   if (DECL_ARGUMENTS (fn))
-    error ("concept %q#D declared with function parameters", fn);
+    error_at (DECL_SOURCE_LOCATION (fn),
+	      "concept %q#D declared with function parameters", fn);
 
   // The declared return type of the concept shall be bool, and
   // it shall not be deduced from it definition.
   tree type = TREE_TYPE (TREE_TYPE (fn));
   if (is_auto (type))
-    error ("concept %q#D declared with a deduced return type", fn);
+    error_at (DECL_SOURCE_LOCATION (fn),
+	      "concept %q#D declared with a deduced return type", fn);
   else if (type != boolean_type_node)
-    error ("concept %q#D with non-%<bool%> return type %qT", fn, type);
+    error_at (DECL_SOURCE_LOCATION (fn),
+	      "concept %q#D with non-%<bool%> return type %qT", fn, type);
 }
 
 /* Helper function.  Replace the temporary this parameter injected
@@ -9795,6 +9798,18 @@ create_array_type_for_decl (tree name, tree type,
   return build_cplus_array_type (type, itype);
 }
 
+/* Returns the smallest location that is not UNKNOWN_LOCATION.  */
+
+static location_t
+min_location (location_t loca, location_t locb)
+{
+  if (loca == UNKNOWN_LOCATION
+      || (locb != UNKNOWN_LOCATION
+	  && linemap_location_before_p (line_table, locb, loca)))
+    return locb;
+  return loca;
+}
+
 /* Returns the smallest location != UNKNOWN_LOCATION among the
    three stored in LOCATIONS[ds_const], LOCATIONS[ds_volatile],
    and LOCATIONS[ds_restrict].  */
@@ -9807,13 +9822,11 @@ smallest_type_quals_location (int type_quals, cons
   if (type_quals & TYPE_QUAL_CONST)
     loc = locations[ds_const];
 
-  if ((type_quals & TYPE_QUAL_VOLATILE)
-      && (loc == UNKNOWN_LOCATION || locations[ds_volatile] < loc))
-    loc = locations[ds_volatile];
+  if (type_quals & TYPE_QUAL_VOLATILE)
+    loc = min_location (loc, locations[ds_volatile]);
 
-  if ((type_quals & TYPE_QUAL_RESTRICT)
-      && (loc == UNKNOWN_LOCATION || locations[ds_restrict] < loc))
-    loc = locations[ds_restrict];
+  if (type_quals & TYPE_QUAL_RESTRICT)
+    loc = min_location (loc, locations[ds_restrict]);
 
   return loc;
 }
@@ -10710,14 +10723,20 @@ grokdeclarator (const cp_declarator *declarator,
     {
       if (staticp == 2)
 	{
-	  error ("member %qD cannot be declared both %<virtual%> "
-		 "and %<static%>", dname);
+	  rich_location richloc (line_table, declspecs->locations[ds_virtual]);
+	  richloc.add_range (declspecs->locations[ds_storage_class], false);
+	  error_at (&richloc, "member %qD cannot be declared both %<virtual%> "
+		    "and %<static%>", dname);
 	  storage_class = sc_none;
 	  staticp = 0;
 	}
       if (constexpr_p)
-	error ("member %qD cannot be declared both %<virtual%> "
-	       "and %<constexpr%>", dname);
+	{
+	  rich_location richloc (line_table, declspecs->locations[ds_virtual]);
+	  richloc.add_range (declspecs->locations[ds_constexpr], false);
+	  error_at (&richloc, "member %qD cannot be declared both %<virtual%> "
+		    "and %<constexpr%>", dname);
+	}
     }
   friendp = decl_spec_seq_has_spec_p (declspecs, ds_friend);
 
@@ -10726,18 +10745,27 @@ grokdeclarator (const cp_declarator *declarator,
     {
       if (typedef_p)
 	{
-	  error ("typedef declaration invalid in parameter declaration");
+	  error_at (declspecs->locations[ds_typedef],
+		    "typedef declaration invalid in parameter declaration");
 	  return error_mark_node;
 	}
       else if (template_parm_flag && storage_class != sc_none)
 	{
-	  error ("storage class specified for template parameter %qs", name);
+	  error_at (min_location (declspecs->locations[ds_thread],
+				  declspecs->locations[ds_storage_class]),
+		    "storage class specified for template parameter %qs",
+		    name);
 	  return error_mark_node;
 	}
       else if (storage_class == sc_static
 	       || storage_class == sc_extern
 	       || thread_p)
-	error ("storage class specifiers invalid in parameter declarations");
+	{
+	  error_at (min_location (declspecs->locations[ds_thread],
+				  declspecs->locations[ds_storage_class]),
+		    "storage class specified for parameter %qs", name);
+	  return error_mark_node;
+	}
 
       /* Function parameters cannot be concept. */
       if (concept_p)
@@ -10871,15 +10899,15 @@ grokdeclarator (const cp_declarator *declarator,
 	;
       else
 	{
+	  location_t loc
+	    = min_location (declspecs->locations[ds_thread],
+			    declspecs->locations[ds_storage_class]);
 	  if (decl_context == FIELD)
-	    error ("storage class specified for %qs", name);
+	    error_at (loc, "storage class specified for %qs", name);
+	  else if (decl_context == PARM || decl_context == CATCHPARM)
+	    error_at (loc, "storage class specified for parameter %qs", name);
 	  else
-	    {
-	      if (decl_context == PARM || decl_context == CATCHPARM)
-		error ("storage class specified for parameter %qs", name);
-	      else
-		error ("storage class specified for typename");
-	    }
+	    error_at (loc, "storage class specified for typename");
 	  if (storage_class == sc_register
 	      || storage_class == sc_auto
 	      || storage_class == sc_extern
@@ -10900,7 +10928,8 @@ grokdeclarator (const cp_declarator *declarator,
 	   && storage_class != sc_static)
     {
       if (declspecs->gnu_thread_keyword_p)
-	pedwarn (input_location, 0, "function-scope %qs implicitly auto and "
+	pedwarn (declspecs->locations[ds_thread],
+		 0, "function-scope %qs implicitly auto and "
 		 "declared %<__thread%>", name);
 
       /* When thread_local is applied to a variable of block scope the
@@ -10912,7 +10941,10 @@ grokdeclarator (const cp_declarator *declarator,
 
   if (storage_class && friendp)
     {
-      error ("storage class specifiers invalid in friend function declarations");
+      error_at (min_location (declspecs->locations[ds_thread],
+			      declspecs->locations[ds_storage_class]),
+		"storage class specifiers invalid in friend function "
+		"declarations");
       storage_class = sc_none;
       staticp = 0;
     }
@@ -11238,7 +11270,8 @@ grokdeclarator (const cp_declarator *declarator,
 		if (virtualp)
 		  {
 		    /* Cannot be both friend and virtual.  */
-		    error ("virtual functions cannot be friends");
+		    error_at (declspecs->locations[ds_friend],
+			      "virtual functions cannot be friends");
 		    friendp = 0;
 		  }
 		if (decl_context == NORMAL)
@@ -12369,15 +12402,18 @@ grokdeclarator (const cp_declarator *declarator,
 	else if (thread_p)
 	  {
 	    if (declspecs->gnu_thread_keyword_p)
-	      error ("storage class %<__thread%> invalid for function %qs",
-		     name);
+	      error_at (declspecs->locations[ds_thread],
+			"storage class %<__thread%> invalid for function %qs",
+			name);
 	    else
-	      error ("storage class %<thread_local%> invalid for function %qs",
-		     name);
+	      error_at (declspecs->locations[ds_thread],
+			"storage class %<thread_local%> invalid for "
+			"function %qs", name);
 	  }
 
         if (virt_specifiers)
-          error ("virt-specifiers in %qs not allowed outside a class definition", name);
+          error ("virt-specifiers in %qs not allowed outside a class "
+		 "definition", name);
 	/* Function declaration not at top level.
 	   Storage classes other than `extern' are not allowed
 	   and `extern' makes no difference.  */
Index: testsuite/g++.dg/concepts/fn-concept2.C
===================================================================
--- testsuite/g++.dg/concepts/fn-concept2.C	(revision 262329)
+++ testsuite/g++.dg/concepts/fn-concept2.C	(working copy)
@@ -1,7 +1,10 @@
 // { dg-options "-std=c++17 -fconcepts" }
 
 template<typename T>
-  concept auto C1() { return 0; } // { dg-error "deduced return type" }
+  concept auto C1() { return 0; } // { dg-error "16:concept .concept auto C1\\(\\). declared with a deduced return type" }
 
 template<typename T>
-  concept int C2() { return 0; } // { dg-error "return type" }
+  concept int C2() { return 0; } // { dg-error "15:concept .concept int C2\\(\\). with non-.bool. return type .int." }
+
+template<typename T>
+  concept bool C3(int) { return 0; } // { dg-error "16:concept .concept bool C3\\(int\\). declared with function parameters" }
Index: testsuite/g++.dg/cpp0x/constexpr-virtual5.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-virtual5.C	(revision 262329)
+++ testsuite/g++.dg/cpp0x/constexpr-virtual5.C	(working copy)
@@ -2,5 +2,5 @@
 // { dg-do compile { target c++11 } }
 
 struct S {
-  constexpr virtual int f() { return 1; }  // { dg-error "both 'virtual' and 'constexpr'" }
+  constexpr virtual int f() { return 1; }  // { dg-error "13:member .f. cannot be declared both .virtual. and .constexpr." }
 };
Index: testsuite/g++.dg/cpp0x/pr51463.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr51463.C	(revision 262329)
+++ testsuite/g++.dg/cpp0x/pr51463.C	(working copy)
@@ -3,5 +3,6 @@
 
 struct A
 {
-  static virtual int i = 0;	// { dg-error "both 'virtual' and 'static'|declared as" }
+  static virtual int i = 0;	// { dg-error "10:member .i. cannot be declared both .virtual. and .static." }
+  // { dg-error "declared as" "" { target *-*-* } .-1 }
 };
Index: testsuite/g++.dg/diagnostic/virtual-constexpr.C
===================================================================
--- testsuite/g++.dg/diagnostic/virtual-constexpr.C	(nonexistent)
+++ testsuite/g++.dg/diagnostic/virtual-constexpr.C	(working copy)
@@ -0,0 +1,16 @@
+// { dg-options "-fdiagnostics-show-caret" }
+// { dg-do compile { target c++11 } }
+
+struct S
+{
+  virtual constexpr void foo();  // { dg-error "3:member .foo. cannot be declared both .virtual. and .constexpr." }
+/* { dg-begin-multiline-output "" }
+   virtual constexpr void foo();
+   ^~~~~~~ ~~~~~~~~~
+   { dg-end-multiline-output "" } */
+  constexpr virtual void bar();  // { dg-error "13:member .bar. cannot be declared both .virtual. and .constexpr." }
+/* { dg-begin-multiline-output "" }
+   constexpr virtual void bar();
+   ~~~~~~~~~ ^~~~~~~
+   { dg-end-multiline-output "" } */
+};
Index: testsuite/g++.dg/diagnostic/virtual-static.C
===================================================================
--- testsuite/g++.dg/diagnostic/virtual-static.C	(nonexistent)
+++ testsuite/g++.dg/diagnostic/virtual-static.C	(working copy)
@@ -0,0 +1,15 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+struct S
+{
+  virtual static void foo();  // { dg-error "3:member .foo. cannot be declared both .virtual. and .static." }
+/* { dg-begin-multiline-output "" }
+   virtual static void foo();
+   ^~~~~~~ ~~~~~~
+   { dg-end-multiline-output "" } */
+  static virtual void bar();  // { dg-error "10:member .bar. cannot be declared both .virtual. and .static." }
+/* { dg-begin-multiline-output "" }
+   static virtual void bar();
+   ~~~~~~ ^~~~~~~
+   { dg-end-multiline-output "" } */
+};
Index: testsuite/g++.dg/other/locations1.C
===================================================================
--- testsuite/g++.dg/other/locations1.C	(nonexistent)
+++ testsuite/g++.dg/other/locations1.C	(working copy)
@@ -0,0 +1 @@
+void foo(static int p);  // { dg-error "10:storage class specified" }
Index: testsuite/g++.dg/other/typedef1.C
===================================================================
--- testsuite/g++.dg/other/typedef1.C	(revision 262329)
+++ testsuite/g++.dg/other/typedef1.C	(working copy)
@@ -1,7 +1,10 @@
 // PR c++/27572
 // { dg-do compile }
 
-void f1(typedef) {}        // { dg-error "no type|typedef declaration" }
-void f2(typedef x) {}      // { dg-error "type|typedef declaration" }
-void f3(typedef x[]) {}    // { dg-error "type|typedef declaration" }
-void f4(typedef int x) {}  // { dg-error "typedef declaration" }
+void f1(typedef) {}        // { dg-error "9:typedef declaration" }
+// { dg-error "no type" "" { target *-*-* } .-1 }
+void f2(typedef x) {}      // { dg-error "9:typedef declaration" }
+// { dg-error "type" "" { target *-*-* } .-1 }
+void f3(typedef x[]) {}    // { dg-error "9:typedef declaration" }
+// { dg-error "type" "" { target *-*-* } .-1 }
+void f4(typedef int x) {}  // { dg-error "9:typedef declaration" }
Index: testsuite/g++.dg/parse/dtor13.C
===================================================================
--- testsuite/g++.dg/parse/dtor13.C	(revision 262329)
+++ testsuite/g++.dg/parse/dtor13.C	(working copy)
@@ -3,6 +3,7 @@
 
 struct A
 {
-  static friend A::~A(); /* { dg-error "storage class specifiers|extra qualification|implicitly friend" } */
+  static friend A::~A(); /* { dg-error "3:storage class specifiers" } */
+  /* { dg-error "extra qualification|implicitly friend" "" { target *-*-* } .-1 } */
 };
 
Index: testsuite/g++.dg/template/error44.C
===================================================================
--- testsuite/g++.dg/template/error44.C	(revision 262329)
+++ testsuite/g++.dg/template/error44.C	(working copy)
@@ -1,7 +1,8 @@
 // PR c++/32056
 
-template <auto int T> struct A {}; // { dg-error "storage class specified|two or more" }
-template <extern int T> struct B {}; // { dg-error "storage class specified" }
-template <static int T> struct C {}; // { dg-error "storage class specified" }
-template <register int T> struct D {}; // { dg-error "storage class specified" }
-template <mutable int T> struct E {}; // { dg-error "storage class specified" }
+template <auto int T> struct A {}; // { dg-error "11:storage class specified" "" { target c++98_only } }
+// { dg-error "two or more" "" { target c++11 } .-1 }
+template <extern int T> struct B {}; // { dg-error "11:storage class specified" }
+template <static int T> struct C {}; // { dg-error "11:storage class specified" }
+template <register int T> struct D {}; // { dg-error "11:storage class specified" }
+template <mutable int T> struct E {}; // { dg-error "11:storage class specified" }
Index: testsuite/g++.dg/template/typedef4.C
===================================================================
--- testsuite/g++.dg/template/typedef4.C	(revision 262329)
+++ testsuite/g++.dg/template/typedef4.C	(working copy)
@@ -1,7 +1,8 @@
 // PR c++/27572
 // { dg-do compile }
 
-template<typedef> void foo();  // { dg-error "no type|typedef declaration|template" }
+template<typedef> void foo();  // { dg-error "10:typedef declaration" }
+// { dg-error "no type|template" "" { target *-*-* } .-1 }
 
 void bar()
 {
Index: testsuite/g++.dg/template/typedef5.C
===================================================================
--- testsuite/g++.dg/template/typedef5.C	(revision 262329)
+++ testsuite/g++.dg/template/typedef5.C	(working copy)
@@ -1,7 +1,10 @@
 // PR c++/27572
 // { dg-do compile }
 
-template<typedef,int>        struct A1; // { dg-error "no type|typedef declaration|default argument" }
-template<typedef x,int>      struct A2; // { dg-error "type|typedef declaration|default argument" }
-template<typedef x[],int>    struct A3; // { dg-error "no type|typedef declaration|expected" }
-template<typedef int x, int> struct A4; // { dg-error "typedef declaration|default argument" }
+template<typedef,int>        struct A1; // { dg-error "10:typedef declaration" }
+// { dg-error "no type|default argument" "" { target *-*-* } .-1 }
+template<typedef x,int>      struct A2; // { dg-error "10:typedef declaration" }
+// { dg-error "type|default argument" "" { target *-*-* } .-1 }
+template<typedef x[],int>    struct A3; // { dg-error "typedef declaration|no type|expected" }
+template<typedef int x, int> struct A4; // { dg-error "10:typedef declaration" }
+// { dg-error "default argument" "" { target *-*-* } .-1 }
Index: testsuite/g++.dg/tls/diag-2.C
===================================================================
--- testsuite/g++.dg/tls/diag-2.C	(revision 262329)
+++ testsuite/g++.dg/tls/diag-2.C	(working copy)
@@ -8,19 +8,19 @@ typedef __thread int g4;	/* { dg-error "multiple s
 
 void foo()
 {
-  __thread int l1;		/* { dg-error "implicitly auto and declared '__thread'" } */
+  __thread int l1;		/* { dg-error "3:function-scope .l1. implicitly auto and declared '__thread'" } */
   auto __thread int l2;		/* { dg-error "multiple storage classes|data types" } */
   __thread extern int l3;	/* { dg-error "'__thread' before 'extern'" } */
   register __thread int l4;	/* { dg-error "multiple storage classes" } */
 }				/* { dg-error "ISO C\\+\\+17 does not allow 'register' storage class specifier" "" { target c++17 } .-1 } */
 
-__thread void f1 ();		/* { dg-error "invalid for function" } */
-extern __thread void f2 ();	/* { dg-error "invalid for function" } */
-static __thread void f3 ();	/* { dg-error "invalid for function" } */
-__thread void f4 () { }		/* { dg-error "invalid for function" } */
+__thread void f1 ();		/* { dg-error "1:storage class .__thread. invalid for function" } */
+extern __thread void f2 ();	/* { dg-error "8:storage class .__thread. invalid for function" } */
+static __thread void f3 ();	/* { dg-error "8:storage class .__thread. invalid for function" } */
+__thread void f4 () { }		/* { dg-error "1:storage class .__thread. invalid for function" } */
 
-void bar(__thread int p1);	/* { dg-error "(invalid in parameter)|(specified for parameter)" } */
+void bar(__thread int p1);	/* { dg-error "10:storage class specified for parameter" } */
 
 struct A {
-  __thread int i;		/* { dg-error "storage class specified" } */
+  __thread int i;		/* { dg-error "3:storage class specified" } */
 };
Index: testsuite/g++.dg/tls/locations1.C
===================================================================
--- testsuite/g++.dg/tls/locations1.C	(nonexistent)
+++ testsuite/g++.dg/tls/locations1.C	(working copy)
@@ -0,0 +1,4 @@
+/* { dg-require-effective-target tls } */
+
+template <__thread int T> struct F {}; // { dg-error "11:storage class specified" }
+template <static __thread int T> struct G {}; // { dg-error "11:storage class specified" }
Index: testsuite/g++.old-deja/g++.brendan/crash11.C
===================================================================
--- testsuite/g++.old-deja/g++.brendan/crash11.C	(revision 262329)
+++ testsuite/g++.old-deja/g++.brendan/crash11.C	(working copy)
@@ -9,13 +9,14 @@ class A {
 	int	h;
 	A() { i=10; j=20; }
 	virtual void f1() { printf("i=%d j=%d\n",i,j); }
-	friend virtual void f2() { printf("i=%d j=%d\n",i,j); }// { dg-error "" }  virtual.*
+	friend virtual void f2() { printf("i=%d j=%d\n",i,j); } // { dg-error "2:virtual functions cannot be friends" }
 };
 
 class B : public A {
     public:
 	virtual void f1() { printf("i=%d j=%d\n",i,j); }// { dg-error "" }  member.*// ERROR -  member.*
-	friend virtual void f2() { printf("i=%d j=%d\n",i,j); }// { dg-error "" }  virtual.*// ERROR -  member.*// ERROR -  member.*
+	friend virtual void f2() { printf("i=%d j=%d\n",i,j); }  // { dg-error "2:virtual functions cannot be friends" }
+// { dg-error "private" "" { target *-*-* } .-1 }
 };
 
 int
Jason Merrill July 3, 2018, 8:28 p.m. | #10
OK.

On Tue, Jul 3, 2018 at 2:37 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,

>

> On 03/07/2018 18:36, Jason Merrill wrote:

>>

>>

>>>     if ((type_quals & TYPE_QUAL_VOLATILE)

>>> -      && (loc == UNKNOWN_LOCATION || locations[ds_volatile] < loc))

>>> +      && (loc == UNKNOWN_LOCATION

>>> +          || linemap_location_before_p (line_table,

>>> +                                        locations[ds_volatile], loc)))

>>>       loc = locations[ds_volatile];

>>>     if ((type_quals & TYPE_QUAL_RESTRICT)

>>> -      && (loc == UNKNOWN_LOCATION || locations[ds_restrict] < loc))

>>> +      && (loc == UNKNOWN_LOCATION

>>> +          || linemap_location_before_p (line_table,

>>> +                                        locations[ds_restrict], loc)))

>>>       loc = locations[ds_restrict];

>>

>> Why not use min_location here?

>

> Indeed. Thus I successfully tested the below.

>

> Thanks,

> Paolo.

>

> //////////////////////

Patch

Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 262005)
+++ cp/decl.c	(working copy)
@@ -8545,15 +8545,18 @@  check_concept_fn (tree fn)
 {
   // A constraint is nullary.
   if (DECL_ARGUMENTS (fn))
-    error ("concept %q#D declared with function parameters", fn);
+    error_at (DECL_SOURCE_LOCATION (fn),
+	      "concept %q#D declared with function parameters", fn);
 
   // The declared return type of the concept shall be bool, and
   // it shall not be deduced from it definition.
   tree type = TREE_TYPE (TREE_TYPE (fn));
   if (is_auto (type))
-    error ("concept %q#D declared with a deduced return type", fn);
+    error_at (DECL_SOURCE_LOCATION (fn),
+	      "concept %q#D declared with a deduced return type", fn);
   else if (type != boolean_type_node)
-    error ("concept %q#D with non-%<bool%> return type %qT", fn, type);
+    error_at (DECL_SOURCE_LOCATION (fn),
+	      "concept %q#D with non-%<bool%> return type %qT", fn, type);
 }
 
 /* Helper function.  Replace the temporary this parameter injected
@@ -9818,6 +9821,27 @@  smallest_type_quals_location (int type_quals, cons
   return loc;
 }
 
+/* Returns the smallest location.  */
+
+static location_t
+min_location (location_t loca, location_t locb)
+{
+  if (loca == UNKNOWN_LOCATION
+      || (locb != UNKNOWN_LOCATION && locb < loca))
+    return locb;
+  return loca;
+}
+
+/* Returns the biggest location.  */
+
+static location_t
+max_location (location_t loca, location_t locb)
+{
+  if (loca < locb)
+    return locb;
+  return loca;
+}
+
 /* Check that it's OK to declare a function with the indicated TYPE
    and TYPE_QUALS.  SFK indicates the kind of special function (if any)
    that this function is.  OPTYPE is the type given in a conversion
@@ -10710,14 +10734,18 @@  grokdeclarator (const cp_declarator *declarator,
     {
       if (staticp == 2)
 	{
-	  error ("member %qD cannot be declared both %<virtual%> "
-		 "and %<static%>", dname);
+	  error_at (max_location (declspecs->locations[ds_virtual],
+				  declspecs->locations[ds_storage_class]),
+		    "member %qD cannot be declared both %<virtual%> "
+		    "and %<static%>", dname);
 	  storage_class = sc_none;
 	  staticp = 0;
 	}
       if (constexpr_p)
-	error ("member %qD cannot be declared both %<virtual%> "
-	       "and %<constexpr%>", dname);
+	error_at (max_location (declspecs->locations[ds_virtual],
+				declspecs->locations[ds_constexpr]),
+		  "member %qD cannot be declared both %<virtual%> "
+		  "and %<constexpr%>", dname);
     }
   friendp = decl_spec_seq_has_spec_p (declspecs, ds_friend);
 
@@ -10726,18 +10754,27 @@  grokdeclarator (const cp_declarator *declarator,
     {
       if (typedef_p)
 	{
-	  error ("typedef declaration invalid in parameter declaration");
+	  error_at (declspecs->locations[ds_typedef],
+		    "typedef declaration invalid in parameter declaration");
 	  return error_mark_node;
 	}
       else if (template_parm_flag && storage_class != sc_none)
 	{
-	  error ("storage class specified for template parameter %qs", name);
+	  error_at (min_location (declspecs->locations[ds_thread],
+				  declspecs->locations[ds_storage_class]),
+		    "storage class specified for template parameter %qs",
+		    name);
 	  return error_mark_node;
 	}
       else if (storage_class == sc_static
 	       || storage_class == sc_extern
 	       || thread_p)
-	error ("storage class specifiers invalid in parameter declarations");
+	{
+	  error_at (min_location (declspecs->locations[ds_thread],
+				  declspecs->locations[ds_storage_class]),
+		    "storage class specified for parameter %qs", name);
+	  return error_mark_node;
+	}
 
       /* Function parameters cannot be concept. */
       if (concept_p)
@@ -10872,13 +10909,19 @@  grokdeclarator (const cp_declarator *declarator,
       else
 	{
 	  if (decl_context == FIELD)
-	    error ("storage class specified for %qs", name);
+	    error_at (min_location (declspecs->locations[ds_thread],
+				    declspecs->locations[ds_storage_class]),
+		      "storage class specified for %qs", name);
 	  else
 	    {
 	      if (decl_context == PARM || decl_context == CATCHPARM)
-		error ("storage class specified for parameter %qs", name);
+		error_at (min_location (declspecs->locations[ds_thread],
+					declspecs->locations[ds_storage_class]),
+			  "storage class specified for parameter %qs", name);
 	      else
-		error ("storage class specified for typename");
+		error_at (min_location (declspecs->locations[ds_thread],
+					declspecs->locations[ds_storage_class]),
+			  "storage class specified for typename");
 	    }
 	  if (storage_class == sc_register
 	      || storage_class == sc_auto
@@ -10900,7 +10943,8 @@  grokdeclarator (const cp_declarator *declarator,
 	   && storage_class != sc_static)
     {
       if (declspecs->gnu_thread_keyword_p)
-	pedwarn (input_location, 0, "function-scope %qs implicitly auto and "
+	pedwarn (declspecs->locations[ds_thread],
+		 0, "function-scope %qs implicitly auto and "
 		 "declared %<__thread%>", name);
 
       /* When thread_local is applied to a variable of block scope the
@@ -10912,7 +10956,10 @@  grokdeclarator (const cp_declarator *declarator,
 
   if (storage_class && friendp)
     {
-      error ("storage class specifiers invalid in friend function declarations");
+      error_at (min_location (declspecs->locations[ds_thread],
+			      declspecs->locations[ds_storage_class]),
+		"storage class specifiers invalid in friend function "
+		"declarations");
       storage_class = sc_none;
       staticp = 0;
     }
@@ -11238,7 +11285,8 @@  grokdeclarator (const cp_declarator *declarator,
 		if (virtualp)
 		  {
 		    /* Cannot be both friend and virtual.  */
-		    error ("virtual functions cannot be friends");
+		    error_at (declspecs->locations[ds_friend],
+			      "virtual functions cannot be friends");
 		    friendp = 0;
 		  }
 		if (decl_context == NORMAL)
@@ -12369,15 +12417,18 @@  grokdeclarator (const cp_declarator *declarator,
 	else if (thread_p)
 	  {
 	    if (declspecs->gnu_thread_keyword_p)
-	      error ("storage class %<__thread%> invalid for function %qs",
-		     name);
+	      error_at (declspecs->locations[ds_thread],
+			"storage class %<__thread%> invalid for function %qs",
+			name);
 	    else
-	      error ("storage class %<thread_local%> invalid for function %qs",
-		     name);
+	      error_at (declspecs->locations[ds_thread],
+			"storage class %<thread_local%> invalid for "
+			"function %qs", name);
 	  }
 
         if (virt_specifiers)
-          error ("virt-specifiers in %qs not allowed outside a class definition", name);
+          error ("virt-specifiers in %qs not allowed outside a class "
+		 "definition", name);
 	/* Function declaration not at top level.
 	   Storage classes other than `extern' are not allowed
 	   and `extern' makes no difference.  */
Index: testsuite/g++.dg/concepts/fn-concept2.C
===================================================================
--- testsuite/g++.dg/concepts/fn-concept2.C	(revision 262005)
+++ testsuite/g++.dg/concepts/fn-concept2.C	(working copy)
@@ -1,7 +1,10 @@ 
 // { dg-options "-std=c++17 -fconcepts" }
 
 template<typename T>
-  concept auto C1() { return 0; } // { dg-error "deduced return type" }
+  concept auto C1() { return 0; } // { dg-error "16:concept .concept auto C1\\(\\). declared with a deduced return type" }
 
 template<typename T>
-  concept int C2() { return 0; } // { dg-error "return type" }
+  concept int C2() { return 0; } // { dg-error "15:concept .concept int C2\\(\\). with non-.bool. return type .int." }
+
+template<typename T>
+  concept bool C3(int) { return 0; } // { dg-error "16:concept .concept bool C3\\(int\\). declared with function parameters" }
Index: testsuite/g++.dg/cpp0x/constexpr-virtual5.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-virtual5.C	(revision 262005)
+++ testsuite/g++.dg/cpp0x/constexpr-virtual5.C	(working copy)
@@ -2,5 +2,5 @@ 
 // { dg-do compile { target c++11 } }
 
 struct S {
-  constexpr virtual int f() { return 1; }  // { dg-error "both 'virtual' and 'constexpr'" }
+  constexpr virtual int f() { return 1; }  // { dg-error "13:member .f. cannot be declared both .virtual. and .constexpr." }
 };
Index: testsuite/g++.dg/cpp0x/pr51463.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr51463.C	(revision 262005)
+++ testsuite/g++.dg/cpp0x/pr51463.C	(working copy)
@@ -3,5 +3,5 @@ 
 
 struct A
 {
-  static virtual int i = 0;	// { dg-error "both 'virtual' and 'static'|declared as" }
+  static virtual int i = 0;	// { dg-error "10:member .i. cannot be declared both .virtual. and .static.|declared as" }
 };
Index: testsuite/g++.dg/other/locations1.C
===================================================================
--- testsuite/g++.dg/other/locations1.C	(nonexistent)
+++ testsuite/g++.dg/other/locations1.C	(working copy)
@@ -0,0 +1 @@ 
+void foo(static int p);  // { dg-error "10:storage class specified" }
Index: testsuite/g++.dg/other/typedef1.C
===================================================================
--- testsuite/g++.dg/other/typedef1.C	(revision 262005)
+++ testsuite/g++.dg/other/typedef1.C	(working copy)
@@ -1,7 +1,7 @@ 
 // PR c++/27572
 // { dg-do compile }
 
-void f1(typedef) {}        // { dg-error "no type|typedef declaration" }
-void f2(typedef x) {}      // { dg-error "type|typedef declaration" }
-void f3(typedef x[]) {}    // { dg-error "type|typedef declaration" }
-void f4(typedef int x) {}  // { dg-error "typedef declaration" }
+void f1(typedef) {}        // { dg-error "9:typedef declaration|no type" }
+void f2(typedef x) {}      // { dg-error "9:typedef declaration|type" }
+void f3(typedef x[]) {}    // { dg-error "9:typedef declaration|type" }
+void f4(typedef int x) {}  // { dg-error "9:typedef declaration" }
Index: testsuite/g++.dg/parse/dtor13.C
===================================================================
--- testsuite/g++.dg/parse/dtor13.C	(revision 262005)
+++ testsuite/g++.dg/parse/dtor13.C	(working copy)
@@ -3,6 +3,6 @@ 
 
 struct A
 {
-  static friend A::~A(); /* { dg-error "storage class specifiers|extra qualification|implicitly friend" } */
+  static friend A::~A(); /* { dg-error "3:storage class specifiers|extra qualification|implicitly friend" } */
 };
 
Index: testsuite/g++.dg/template/error44.C
===================================================================
--- testsuite/g++.dg/template/error44.C	(revision 262005)
+++ testsuite/g++.dg/template/error44.C	(working copy)
@@ -1,7 +1,7 @@ 
 // PR c++/32056
 
-template <auto int T> struct A {}; // { dg-error "storage class specified|two or more" }
-template <extern int T> struct B {}; // { dg-error "storage class specified" }
-template <static int T> struct C {}; // { dg-error "storage class specified" }
-template <register int T> struct D {}; // { dg-error "storage class specified" }
-template <mutable int T> struct E {}; // { dg-error "storage class specified" }
+template <auto int T> struct A {}; // { dg-error "11:storage class specified|two or more" }
+template <extern int T> struct B {}; // { dg-error "11:storage class specified" }
+template <static int T> struct C {}; // { dg-error "11:storage class specified" }
+template <register int T> struct D {}; // { dg-error "11:storage class specified" }
+template <mutable int T> struct E {}; // { dg-error "11:storage class specified" }
Index: testsuite/g++.dg/template/typedef4.C
===================================================================
--- testsuite/g++.dg/template/typedef4.C	(revision 262005)
+++ testsuite/g++.dg/template/typedef4.C	(working copy)
@@ -1,7 +1,7 @@ 
 // PR c++/27572
 // { dg-do compile }
 
-template<typedef> void foo();  // { dg-error "no type|typedef declaration|template" }
+template<typedef> void foo();  // { dg-error "10:typedef declaration|no type|template" }
 
 void bar()
 {
Index: testsuite/g++.dg/template/typedef5.C
===================================================================
--- testsuite/g++.dg/template/typedef5.C	(revision 262005)
+++ testsuite/g++.dg/template/typedef5.C	(working copy)
@@ -1,7 +1,7 @@ 
 // PR c++/27572
 // { dg-do compile }
 
-template<typedef,int>        struct A1; // { dg-error "no type|typedef declaration|default argument" }
-template<typedef x,int>      struct A2; // { dg-error "type|typedef declaration|default argument" }
-template<typedef x[],int>    struct A3; // { dg-error "no type|typedef declaration|expected" }
-template<typedef int x, int> struct A4; // { dg-error "typedef declaration|default argument" }
+template<typedef,int>        struct A1; // { dg-error "10:typedef declaration|no type|default argument" }
+template<typedef x,int>      struct A2; // { dg-error "10:typedef declaration|type|default argument" }
+template<typedef x[],int>    struct A3; // { dg-error "10:typedef declaration|no type|expected" }
+template<typedef int x, int> struct A4; // { dg-error "10:typedef declaration|default argument" }
Index: testsuite/g++.dg/tls/diag-2.C
===================================================================
--- testsuite/g++.dg/tls/diag-2.C	(revision 262005)
+++ testsuite/g++.dg/tls/diag-2.C	(working copy)
@@ -8,19 +8,19 @@  typedef __thread int g4;	/* { dg-error "multiple s
 
 void foo()
 {
-  __thread int l1;		/* { dg-error "implicitly auto and declared '__thread'" } */
+  __thread int l1;		/* { dg-error "3:function-scope .l1. implicitly auto and declared '__thread'" } */
   auto __thread int l2;		/* { dg-error "multiple storage classes|data types" } */
   __thread extern int l3;	/* { dg-error "'__thread' before 'extern'" } */
   register __thread int l4;	/* { dg-error "multiple storage classes" } */
 }				/* { dg-error "ISO C\\+\\+17 does not allow 'register' storage class specifier" "" { target c++17 } .-1 } */
 
-__thread void f1 ();		/* { dg-error "invalid for function" } */
-extern __thread void f2 ();	/* { dg-error "invalid for function" } */
-static __thread void f3 ();	/* { dg-error "invalid for function" } */
-__thread void f4 () { }		/* { dg-error "invalid for function" } */
+__thread void f1 ();		/* { dg-error "1:storage class .__thread. invalid for function" } */
+extern __thread void f2 ();	/* { dg-error "8:storage class .__thread. invalid for function" } */
+static __thread void f3 ();	/* { dg-error "8:storage class .__thread. invalid for function" } */
+__thread void f4 () { }		/* { dg-error "1:storage class .__thread. invalid for function" } */
 
-void bar(__thread int p1);	/* { dg-error "(invalid in parameter)|(specified for parameter)" } */
+void bar(__thread int p1);	/* { dg-error "10:storage class specified for parameter" } */
 
 struct A {
-  __thread int i;		/* { dg-error "storage class specified" } */
+  __thread int i;		/* { dg-error "3:storage class specified" } */
 };
Index: testsuite/g++.dg/tls/locations1.C
===================================================================
--- testsuite/g++.dg/tls/locations1.C	(nonexistent)
+++ testsuite/g++.dg/tls/locations1.C	(working copy)
@@ -0,0 +1,3 @@ 
+/* { dg-require-effective-target tls } */
+
+template <__thread int T> struct F {}; // { dg-error "11:storage class specified" }
Index: testsuite/g++.old-deja/g++.brendan/crash11.C
===================================================================
--- testsuite/g++.old-deja/g++.brendan/crash11.C	(revision 262005)
+++ testsuite/g++.old-deja/g++.brendan/crash11.C	(working copy)
@@ -9,13 +9,14 @@  class A {
 	int	h;
 	A() { i=10; j=20; }
 	virtual void f1() { printf("i=%d j=%d\n",i,j); }
-	friend virtual void f2() { printf("i=%d j=%d\n",i,j); }// { dg-error "" }  virtual.*
+	friend virtual void f2() { printf("i=%d j=%d\n",i,j); } // { dg-error "2:virtual functions cannot be friends" }
 };
 
 class B : public A {
     public:
 	virtual void f1() { printf("i=%d j=%d\n",i,j); }// { dg-error "" }  member.*// ERROR -  member.*
-	friend virtual void f2() { printf("i=%d j=%d\n",i,j); }// { dg-error "" }  virtual.*// ERROR -  member.*// ERROR -  member.*
+	friend virtual void f2() { printf("i=%d j=%d\n",i,j); }  // { dg-error "2:virtual functions cannot be friends|private" }
+
 };
 
 int