[C++ Patch] PR 27211

[C++ Patch] PR 27211

[Paolo Carlini]

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

Hi,

the idea for the fix is very simple: check_classfn currently returns
NULL_TREE both in case of real error and when a declaration is just not
found, per the comment at the beginning of its body. Telling those two
situations apart fixes the PR.

I have also checked that ICC behaves similarly, also for the two amended
testcases.

Tested x86_64-linux, Ok for mainline?

Paolo.

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



[-- Attachment #2: CL_27211 --]
[-- Type: text/plain, Size: 500 bytes --]

/cp
2007-08-14  Paolo Carlini  <pcarlini@suse.de>

	PR c++/27211
	* decl2.c (check_classfn): Return error_mark_node in case of error.
	* decl.c (start_decl): Deal with check_classfn returning
	error_mark_node.
	(grokfndecl): Likewise.
	* pt.c (tsubst_friend_function): Likewise.

/testsuite
2007-08-14  Paolo Carlini  <pcarlini@suse.de>

	PR c++/27211
	* g++.dg/template/error27.C: New.
	* g++.dg/template/error28.C: New.
	* g++.dg/other/pr28304.C: Adjust.
	* g++.old-deja/g++.mike/p811.C: Likewise.

[-- Attachment #3: patch_27211 --]
[-- Type: text/plain, Size: 5700 bytes --]

Index: testsuite/g++.old-deja/g++.mike/p811.C
===================================================================
*** testsuite/g++.old-deja/g++.mike/p811.C	(revision 127334)
--- testsuite/g++.old-deja/g++.mike/p811.C	(working copy)
*************** public:
*** 525,531 ****
  char *
  X::stringify() const  // { dg-error "does not match" }
  {
!     return "stringify";         // { dg-warning "deprecated" }
  }
  
  const char *
--- 525,531 ----
  char *
  X::stringify() const  // { dg-error "does not match" }
  {
!     return "stringify";
  }
  
  const char *
Index: testsuite/g++.dg/other/pr28304.C
===================================================================
*** testsuite/g++.dg/other/pr28304.C	(revision 127334)
--- testsuite/g++.dg/other/pr28304.C	(working copy)
*************** template<typename T> void A::foo(T) {}  
*** 7,11 ****
  
  void bar()
  {
!     A::foo(1); // { dg-error "no matching function for call" }
  }
--- 7,11 ----
  
  void bar()
  {
!     A::foo(1); // { dg-error "not a member" }
  }
Index: testsuite/g++.dg/template/error27.C
===================================================================
*** testsuite/g++.dg/template/error27.C	(revision 0)
--- testsuite/g++.dg/template/error27.C	(revision 0)
***************
*** 0 ****
--- 1,5 ----
+ // PR c++/27211
+ 
+ struct A {};
+ 
+ template<int> void A::foo() {} // { dg-error "member function" }
Index: testsuite/g++.dg/template/error28.C
===================================================================
*** testsuite/g++.dg/template/error28.C	(revision 0)
--- testsuite/g++.dg/template/error28.C	(revision 0)
***************
*** 0 ****
--- 1,5 ----
+ // PR c++/27211
+ 
+ struct A {};
+ 
+ template<int> void A::foo(); // { dg-error "member function" }
Index: cp/decl.c
===================================================================
*** cp/decl.c	(revision 127334)
--- cp/decl.c	(working copy)
*************** start_decl (const cp_declarator *declara
*** 3938,3945 ****
  				       > template_class_depth (context))
  				      ? current_template_parms
  				      : NULL_TREE);
! 	  if (field && duplicate_decls (decl, field,
! 					/*newdecl_is_friend=*/false))
  	    decl = field;
  	}
  
--- 3938,3946 ----
  				       > template_class_depth (context))
  				      ? current_template_parms
  				      : NULL_TREE);
! 	  if (field && field != error_mark_node
! 	      && duplicate_decls (decl, field,
! 				 /*newdecl_is_friend=*/false))
  	    decl = field;
  	}
  
*************** grokfndecl (tree ctype,
*** 6365,6377 ****
        && (! TYPE_FOR_JAVA (ctype) || check_java_method (decl))
        && check)
      {
!       tree old_decl;
  
-       old_decl = check_classfn (ctype, decl,
- 				(processing_template_decl
- 				 > template_class_depth (ctype))
- 				? current_template_parms
- 				: NULL_TREE);
        if (old_decl)
  	{
  	  tree ok;
--- 6366,6380 ----
        && (! TYPE_FOR_JAVA (ctype) || check_java_method (decl))
        && check)
      {
!       tree old_decl = check_classfn (ctype, decl,
! 				     (processing_template_decl
! 				      > template_class_depth (ctype))
! 				     ? current_template_parms
! 				     : NULL_TREE);
! 
!       if (old_decl == error_mark_node)
! 	return NULL_TREE;
  
        if (old_decl)
  	{
  	  tree ok;
Index: cp/pt.c
===================================================================
*** cp/pt.c	(revision 127334)
--- cp/pt.c	(working copy)
*************** tsubst_friend_function (tree decl, tree 
*** 6287,6293 ****
  	  tree fn = check_classfn (context,
  				   new_friend, NULL_TREE);
  
! 	  if (fn)
  	    new_friend = fn;
  	}
      }
--- 6287,6293 ----
  	  tree fn = check_classfn (context,
  				   new_friend, NULL_TREE);
  
! 	  if (fn && fn != error_mark_node)
  	    new_friend = fn;
  	}
      }
Index: cp/decl2.c
===================================================================
*** cp/decl2.c	(revision 127395)
--- cp/decl2.c	(working copy)
*************** check_java_method (tree method)
*** 540,547 ****
     TEMPLATE_DECL, it can be NULL since the parameters can be extracted
     from the declaration. If the function is not a function template, it
     must be NULL.
!    It returns the original declaration for the function, or NULL_TREE
!    if no declaration was found (and an error was emitted).  */
  
  tree
  check_classfn (tree ctype, tree function, tree template_parms)
--- 540,547 ----
     TEMPLATE_DECL, it can be NULL since the parameters can be extracted
     from the declaration. If the function is not a function template, it
     must be NULL.
!    It returns the original declaration for the function, NULL_TREE if
!    no declaration was found, error_mark_node if an error was emitted.  */
  
  tree
  check_classfn (tree ctype, tree function, tree template_parms)
*************** check_classfn (tree ctype, tree function
*** 677,692 ****
      error ("no %q#D member function declared in class %qT",
  	   function, ctype);
  
-   /* If we did not find the method in the class, add it to avoid
-      spurious errors (unless the CTYPE is not yet defined, in which
-      case we'll only confuse ourselves when the function is declared
-      properly within the class.  */
-   if (COMPLETE_TYPE_P (ctype))
-     add_method (ctype, function, NULL_TREE);
-   
    if (pushed_scope)
      pop_scope (pushed_scope);
!   return NULL_TREE;
  }
  
  /* DECL is a function with vague linkage.  Remember it so that at the
--- 677,685 ----
      error ("no %q#D member function declared in class %qT",
  	   function, ctype);
  
    if (pushed_scope)
      pop_scope (pushed_scope);
!   return error_mark_node;
  }
  
  /* DECL is a function with vague linkage.  Remember it so that at the

Re: [C++ Patch] PR 27211

[Mark Mitchell]

Paolo Carlini wrote:

> the idea for the fix is very simple: check_classfn currently returns
> NULL_TREE both in case of real error and when a declaration is just not
> found, per the comment at the beginning of its body. Telling those two
> situations apart fixes the PR.

The part of your patch that you didn't 'fess up to :-) is this bit:

-   /* If we did not find the method in the class, add it to avoid
-      spurious errors (unless the CTYPE is not yet defined, in which
-      case we'll only confuse ourselves when the function is declared
-      properly within the class.  */
-   if (COMPLETE_TYPE_P (ctype))
-     add_method (ctype, function, NULL_TREE);

That code was trying to handle this kind of thing:

  struct S {
  };

  void S::foo() { ... }

  void g() { S s; s.foo(); } // Avoid "spurious" error here.

However, I've always found the idea of stuffing methods into the class
after the fact unattractive.  It might help with error recovery, but it
goes against the core principle that we should try to keep the internal
representation valid and internally consistent.  For example, we've
already determined vtables and overriders by the point of hitting S::foo
above, and I bet we're not magically updating all of that.

So, I'm OK with this change; I just wanted to explicitly admit that we
were doing it.

The patch is OK, thanks.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [C++ Patch] PR 27211

[Paolo Carlini]

Hi Mark,

>Paolo Carlini wrote:
>
>>the idea for the fix is very simple: check_classfn currently returns
>>NULL_TREE both in case of real error and when a declaration is just not
>>found, per the comment at the beginning of its body. Telling those two
>>situations apart fixes the PR.
>>    
>>
>The part of your patch that you didn't 'fess up to :-) is this bit:
>
>-   /* If we did not find the method in the class, add it to avoid
>-      spurious errors (unless the CTYPE is not yet defined, in which
>-      case we'll only confuse ourselves when the function is declared
>-      properly within the class.  */
>-   if (COMPLETE_TYPE_P (ctype))
>-     add_method (ctype, function, NULL_TREE);
>  
>
touché! ;)

>That code was trying to handle this kind of thing:
>
>  struct S {
>  };
>
>  void S::foo() { ... }
>
>  void g() { S s; s.foo(); } // Avoid "spurious" error here.
>
>However, I've always found the idea of stuffing methods into the class
>after the fact unattractive.  It might help with error recovery, but it
>goes against the core principle that we should try to keep the internal
>representation valid and internally consistent.  For example, we've
>already determined vtables and overriders by the point of hitting S::foo
>above, and I bet we're not magically updating all of that.
>  
>
and I totally agree ;) In my experiments, removing that hunk alone 
suffices to cut to half  (e.g., no "candidates" lines) the "junk" that 
we were emitting for test error28.C, thus demonstrating that such 
inconsistent representation can certainly backfire (likewise, the 
diagnostic of pr28304.C, changes (to the better in my opinion) without 
that hunk). Whether in some circumstances that kind of trick can be an 
overall win, I don't know, but as a matter of principle I totally I 
agree with you, as I said.

>So, I'm OK with this change; I just wanted to explicitly admit that we
>were doing it.
>
>The patch is OK, thanks.
>  
>
Thank you,
Paolo.