diff mbox series

[C++] PR 59480 ("Missing error diagnostic: friend declaration specifying a default argument must be a definition")​ (Take 2)

Message ID 562e7947-7f88-a40b-55b4-96c7c1fdfd76@oracle.com
State New
Headers show
Series [C++] PR 59480 ("Missing error diagnostic: friend declaration specifying a default argument must be a definition")​ (Take 2) | expand

Commit Message

Paolo Carlini Aug. 1, 2018, 10:38 a.m. UTC 
Hi,

thus, as you may or may not have noticed I reverted my first try, when 
Tobias noticed that in his large codebase we were rejecting code like:

class Matrix;

Matrix rot90 (const Matrix& a, int k = 1);

class Matrix {
   friend Matrix rot90 (const Matrix&, int);
};

Matrix rot90 (const Matrix& a, int k) { return Matrix(); }

this was happening because, when duplicate_decls saw the friend 
declaration, smashed together the first two rot90 declaration and we 
ended up with a friend declaration with defaults (taken from the first 
rot90 declaration), as such rejected the third time we saw rot90. I 
think we can elegantly handle this kind of problem by exploiting the 
DECL_HIDDEN_FRIEND_P information, thus, in 
check_no_redeclaration_friend_default_args, as called by 
duplicate_decls, if the newdecl doesn't have DECL_FRIEND_P set, 
disregard an olddecl which doesn't have DECL_HIDDEN_FRIEND_P set (we 
can't do this directly because duplicate_decls is already smashing decls 
together at that point, thus we need to save the info and pass it as an 
argument). I added quite a few additional tests an also asked Tobias to 
double check on his code base. All good.

As a general arguments of why I think moving from DECL_FRIEND_P to 
DECL_HIDDEN_FRIEND_P for the olddecl is the right thing to do, if the 
olddecl is a friend but doesn't have DECL_HIDDEN_FRIEND_P set, there was 
a declaration preceding it, thus, either the friend declaration has 
default arguments and we already diagnosed that, or it doesn't , thus 
isn't interesting anymore for the diagnostic at issue.

Thanks! Paolo.

///////////////////////////
/cp
2019-08-01  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/59480, DR 136
	* decl.c (check_no_redeclaration_friend_default_args): New.
	(duplicate_decls): Use the latter; also check that a friend
	declaration specifying default arguments is a definition.

/testsuite
2019-08-01  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/59480, DR 136
	* g++.dg/other/friend8.C: New.
	* g++.dg/other/friend9.C: Likewise.
	* g++.dg/other/friend10.C: Likewise.
	* g++.dg/other/friend11.C: Likewise.
	* g++.dg/other/friend12.C: Likewise.
	* g++.dg/other/friend13.C: Likewise.
	* g++.dg/other/friend14.C: Likewise.
	* g++.dg/other/friend15.C: Likewise.
	* g++.dg/parse/defarg4.C: Compile with -fpermissive -w.
	* g++.dg/parse/defarg8.C: Likewise.

Comments

Jason Merrill Aug. 7, 2018, 11:50 a.m. UTC | #1 
OK.


On Wed, Aug 1, 2018 at 8:38 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> thus, as you may or may not have noticed I reverted my first try, when
> Tobias noticed that in his large codebase we were rejecting code like:
>
> class Matrix;
>
> Matrix rot90 (const Matrix& a, int k = 1);
>
> class Matrix {
>   friend Matrix rot90 (const Matrix&, int);
> };
>
> Matrix rot90 (const Matrix& a, int k) { return Matrix(); }
>
> this was happening because, when duplicate_decls saw the friend declaration,
> smashed together the first two rot90 declaration and we ended up with a
> friend declaration with defaults (taken from the first rot90 declaration),
> as such rejected the third time we saw rot90. I think we can elegantly
> handle this kind of problem by exploiting the DECL_HIDDEN_FRIEND_P
> information, thus, in check_no_redeclaration_friend_default_args, as called
> by duplicate_decls, if the newdecl doesn't have DECL_FRIEND_P set, disregard
> an olddecl which doesn't have DECL_HIDDEN_FRIEND_P set (we can't do this
> directly because duplicate_decls is already smashing decls together at that
> point, thus we need to save the info and pass it as an argument). I added
> quite a few additional tests an also asked Tobias to double check on his
> code base. All good.
>
> As a general arguments of why I think moving from DECL_FRIEND_P to
> DECL_HIDDEN_FRIEND_P for the olddecl is the right thing to do, if the
> olddecl is a friend but doesn't have DECL_HIDDEN_FRIEND_P set, there was a
> declaration preceding it, thus, either the friend declaration has default
> arguments and we already diagnosed that, or it doesn't , thus isn't
> interesting anymore for the diagnostic at issue.
>
> Thanks! Paolo.
>
> ///////////////////////////
diff mbox series

Patch

Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 263197)
+++ cp/decl.c	(working copy)
@@ -1280,6 +1280,38 @@  check_redeclaration_no_default_args (tree decl)
       }
 }
 
+/* NEWDECL is a redeclaration of a function or function template OLDDECL,
+   in any case represented as FUNCTION_DECLs (the DECL_TEMPLATE_RESULTs of
+   the TEMPLATE_DECLs in case of function templates).  This function is used
+   to enforce the final part of C++17 11.3.6/4, about a single declaration:
+   "If a friend declaration specifies a default argument expression, that
+   declaration shall be a definition and shall be the only declaration of
+   the function or function template in the translation unit."  */
+
+static void
+check_no_redeclaration_friend_default_args (tree olddecl, tree newdecl,
+					    bool olddecl_hidden_friend_p)
+{
+  if (!olddecl_hidden_friend_p && !DECL_FRIEND_P (newdecl))
+    return;
+
+  tree t1 = FUNCTION_FIRST_USER_PARMTYPE (olddecl);
+  tree t2 = FUNCTION_FIRST_USER_PARMTYPE (newdecl);
+
+  for (; t1 && t1 != void_list_node;
+       t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
+    if ((olddecl_hidden_friend_p && TREE_PURPOSE (t1))
+	|| (DECL_FRIEND_P (newdecl) && TREE_PURPOSE (t2)))
+      {
+	if (permerror (DECL_SOURCE_LOCATION (newdecl),
+		       "friend declaration of %q#D specifies default "
+		       "arguments and isn't the only declaration", newdecl))
+	  inform (DECL_SOURCE_LOCATION (olddecl),
+		  "previous declaration of %q#D", olddecl);
+	return;
+      }
+}
+
 /* Merge tree bits that correspond to attributes noreturn, nothrow,
    const,  malloc, and pure from NEWDECL with those of OLDDECL.  */
 
@@ -1318,6 +1350,7 @@  duplicate_decls (tree newdecl, tree olddecl, bool
 {
   unsigned olddecl_uid = DECL_UID (olddecl);
   int olddecl_friend = 0, types_match = 0, hidden_friend = 0;
+  int olddecl_hidden_friend = 0;
   int new_defines_function = 0;
   tree new_template_info;
   location_t olddecl_loc = DECL_SOURCE_LOCATION (olddecl);
@@ -1876,6 +1909,13 @@  next_arg:;
 				olddecl);
 		      }
 		  }
+
+	      /* C++17 11.3.6/4: "If a friend declaration specifies a default
+		 argument expression, that declaration... shall be the only
+		 declaration of the function or function template in the
+		 translation unit."  */
+	      check_no_redeclaration_friend_default_args
+		(olddecl, newdecl, DECL_HIDDEN_FRIEND_P (olddecl));
 	    }
 	}
     }
@@ -1982,6 +2022,7 @@  next_arg:;
   if (DECL_DECLARES_FUNCTION_P (olddecl) && DECL_DECLARES_FUNCTION_P (newdecl))
     {
       olddecl_friend = DECL_FRIEND_P (olddecl);
+      olddecl_hidden_friend = DECL_HIDDEN_FRIEND_P (olddecl);
       hidden_friend = (DECL_ANTICIPATED (olddecl)
 		       && DECL_HIDDEN_FRIEND_P (olddecl)
 		       && newdecl_is_friend);
@@ -1994,10 +2035,8 @@  next_arg:;
 
   if (TREE_CODE (newdecl) == TEMPLATE_DECL)
     {
-      tree old_result;
-      tree new_result;
-      old_result = DECL_TEMPLATE_RESULT (olddecl);
-      new_result = DECL_TEMPLATE_RESULT (newdecl);
+      tree old_result = DECL_TEMPLATE_RESULT (olddecl);
+      tree new_result = DECL_TEMPLATE_RESULT (newdecl);
       TREE_TYPE (olddecl) = TREE_TYPE (old_result);
       DECL_TEMPLATE_SPECIALIZATIONS (olddecl)
 	= chainon (DECL_TEMPLATE_SPECIALIZATIONS (olddecl),
@@ -2008,11 +2047,19 @@  next_arg:;
 
       if (DECL_FUNCTION_TEMPLATE_P (newdecl))
 	{
-	  /* Per C++11 8.3.6/4, default arguments cannot be added in later
-	     declarations of a function template.  */
 	  if (DECL_SOURCE_LOCATION (newdecl)
 	      != DECL_SOURCE_LOCATION (olddecl))
-	    check_redeclaration_no_default_args (newdecl);
+	    {
+	      /* Per C++11 8.3.6/4, default arguments cannot be added in
+		 later declarations of a function template.  */
+	      check_redeclaration_no_default_args (newdecl);
+	      /* C++17 11.3.6/4: "If a friend declaration specifies a default
+		 argument expression, that declaration... shall be the only
+		 declaration of the function or function template in the
+		 translation unit."  */
+	      check_no_redeclaration_friend_default_args
+		(old_result, new_result, olddecl_hidden_friend);
+	    }
 
 	  check_default_args (newdecl);
 
@@ -8780,6 +8827,21 @@  grokfndecl (tree ctype,
 	}
     }
 
+  /* C++17 11.3.6/4: "If a friend declaration specifies a default argument
+     expression, that declaration shall be a definition..."  */
+  if (friendp && !funcdef_flag)
+    {
+      for (tree t = FUNCTION_FIRST_USER_PARMTYPE (decl);
+	   t && t != void_list_node; t = TREE_CHAIN (t))
+	if (TREE_PURPOSE (t))
+	  {
+	    permerror (DECL_SOURCE_LOCATION (decl),
+		       "friend declaration of %qD specifies default "
+		       "arguments and isn't a definition", decl);
+	    break;
+	  }
+    }
+
   /* If this decl has namespace scope, set that up.  */
   if (in_namespace)
     set_decl_namespace (decl, in_namespace, friendp);
Index: testsuite/g++.dg/other/friend10.C
===================================================================
--- testsuite/g++.dg/other/friend10.C	(nonexistent)
+++ testsuite/g++.dg/other/friend10.C	(working copy)
@@ -0,0 +1,9 @@ 
+// PR c++/59480
+
+class test {
+  friend int foo(bool = true) { return 1; }  // { dg-message "14:previous" }
+  template<typename> friend int bar(bool = true) { return 1; }  // { dg-message "33:previous" }
+};
+
+int foo(bool);  // { dg-error "5:friend declaration" }
+template<typename> int bar(bool);  // { dg-error "24:friend declaration" }
Index: testsuite/g++.dg/other/friend11.C
===================================================================
--- testsuite/g++.dg/other/friend11.C	(nonexistent)
+++ testsuite/g++.dg/other/friend11.C	(working copy)
@@ -0,0 +1,8 @@ 
+// PR c++/59480
+
+class test {
+  friend int foo(bool = true) { return 1; }  // { dg-message "14:previous" }
+  friend int foo(bool);  // { dg-error "14:friend declaration" }
+  template<typename> friend int bar(bool = true) { return 1; }  // { dg-message "33:previous" }
+  template<typename> friend int bar(bool);  // { dg-error "33:friend declaration" }
+};
Index: testsuite/g++.dg/other/friend12.C
===================================================================
--- testsuite/g++.dg/other/friend12.C	(nonexistent)
+++ testsuite/g++.dg/other/friend12.C	(working copy)
@@ -0,0 +1,11 @@ 
+// PR c++/59480
+
+template<typename>
+class test {
+  friend int foo(bool = true) { return 1; }  // { dg-message "14:previous" }
+  friend int foo(bool);  // { dg-error "14:friend declaration" }
+  template<typename> friend int bar(bool = true) { return 1; }  // { dg-message "33:previous" }
+  template<typename> friend int bar(bool);  // { dg-error "33:friend declaration" }
+};
+
+template class test<bool>;
Index: testsuite/g++.dg/other/friend13.C
===================================================================
--- testsuite/g++.dg/other/friend13.C	(nonexistent)
+++ testsuite/g++.dg/other/friend13.C	(working copy)
@@ -0,0 +1,6 @@ 
+// PR c++/59480
+
+void f(int, int, int=0);  // { dg-message "6:previous" }
+class C {
+  friend void f(int, int=0, int) {}  // { dg-error "15:friend declaration" }
+};
Index: testsuite/g++.dg/other/friend14.C
===================================================================
--- testsuite/g++.dg/other/friend14.C	(nonexistent)
+++ testsuite/g++.dg/other/friend14.C	(working copy)
@@ -0,0 +1,14 @@ 
+// PR c++/59480
+
+class Matrix;
+
+Matrix rot90 (const Matrix& a, int k = 1);
+template<typename> Matrix rot90_ (const Matrix& a, int k = 1);
+
+class Matrix {
+  friend Matrix rot90 (const Matrix&, int);
+  template<typename> friend Matrix rot90_ (const Matrix&, int);
+};
+
+Matrix rot90 (const Matrix& a, int k) { return Matrix(); }
+template<typename> Matrix rot90_ (const Matrix& a, int k) { return Matrix(); }
Index: testsuite/g++.dg/other/friend15.C
===================================================================
--- testsuite/g++.dg/other/friend15.C	(nonexistent)
+++ testsuite/g++.dg/other/friend15.C	(working copy)
@@ -0,0 +1,14 @@ 
+// PR c++/59480
+
+class Matrix;
+
+void rot90 (const Matrix& a, int k = 1) { }
+template<typename> void rot90_ (const Matrix& a, int k = 1) { }
+
+class Matrix {
+  friend void rot90 (const Matrix&, int);
+  template<typename> friend void rot90_ (const Matrix&, int);
+};
+
+void rot90 (const Matrix& a, int k);
+template<typename> void rot90_ (const Matrix& a, int k);
Index: testsuite/g++.dg/other/friend8.C
===================================================================
--- testsuite/g++.dg/other/friend8.C	(nonexistent)
+++ testsuite/g++.dg/other/friend8.C	(working copy)
@@ -0,0 +1,6 @@ 
+// PR c++/59480
+
+class test {
+  friend int foo(bool = true);  // { dg-error "14:friend declaration" }
+  template<typename> friend int bar(bool = true);  // { dg-error "33:friend declaration" }
+};
Index: testsuite/g++.dg/other/friend9.C
===================================================================
--- testsuite/g++.dg/other/friend9.C	(nonexistent)
+++ testsuite/g++.dg/other/friend9.C	(working copy)
@@ -0,0 +1,9 @@ 
+// PR c++/59480
+
+template<typename>
+class test {
+  friend int foo(bool = true);  // { dg-error "14:friend declaration" }
+  template<typename> friend int bar(bool = true);  // { dg-error "33:friend declaration" }
+};
+
+template class test<bool>;
Index: testsuite/g++.dg/parse/defarg4.C
===================================================================
--- testsuite/g++.dg/parse/defarg4.C	(revision 263197)
+++ testsuite/g++.dg/parse/defarg4.C	(working copy)
@@ -1,4 +1,4 @@ 
-// { dg-do compile }
+// { dg-options "-fpermissive -w" }
 
 // Copyright (C) 2003 Free Software Foundation, Inc.
 // Contributed by Nathan Sidwell 3 Jul 2003 <nathan@codesourcery.com>
Index: testsuite/g++.dg/parse/defarg8.C
===================================================================
--- testsuite/g++.dg/parse/defarg8.C	(revision 263197)
+++ testsuite/g++.dg/parse/defarg8.C	(working copy)
@@ -1,3 +1,5 @@ 
+// { dg-options "-fpermissive -w" }
+
 struct A {
   static void g(int);
 };
Index: testsuite/g++.old-deja/g++.mike/p784.C
===================================================================
--- testsuite/g++.old-deja/g++.mike/p784.C	(revision 263197)
+++ testsuite/g++.old-deja/g++.mike/p784.C	(working copy)
@@ -1,6 +1,6 @@ 
 // { dg-do assemble  }
 // { dg-require-effective-target ilp32 } */
-// { dg-options "-w" }
+// { dg-options "-w -fpermissive" }
 // prms-id: 784
 
 //# 1 "GctSymbol.GctSymbol.CHMap.cc"