Keep invalid function body as part of the AST

[ogoffart]

Summary

struct XX {
 double foo(invalid_type xx);
};
double XX::foo(invalid_type xx)
{ 
 return 45; 
}

We should keep XX::foo and its function body as part of the AST so tools can still do something with the body even if the definition is wrong.
Inline function would already be kept, but not when they are redeclarations.

[ogoffart]

Ping?

[rsmith]

lib/Sema/SemaDecl.cpp

5044–5045

Can we delete the invalid-decl check entirely here? If it's doing something important, we need to figure out what and make sure we preserve that intent if it's important, but either way it doesn't make a lot of sense to me for this to depend on whether the declaration has a definition.

[ogoffart]

lib/Sema/SemaDecl.cpp

5044–5045

I tried that, but then we have a failure in Sema/function-redecl.c and Sema/predefined-function.c

int eli(float b); // expected-note {{previous declaration is here}} \
int foo() {
  int eli(int (int)); // expected-error {{conflicting types for 'eli'}}
  eli(b); // expected-error{{passing 'int (int)' to parameter of incompatible type 'float'}}
   return 0;
 }

If we keep the invalid declaration, there would not be an error in the call to eli(b)

But is that a behaviour we would change?

[ogoffart]

An alternative patch is uploaded there: http://reviews.llvm.org/D19764

[ogoffart]

Is this better than the alternative http://reviews.llvm.org/D19764 ?

[ogoffart]

Replaced by http://reviews.llvm.org/D19764

lib/Sema/SemaDecl.cpp

 
   // If this has an identifier and is not an invalid redeclaration or 
   // function template specialization, add it to the scope stack.
-  if (New->getDeclName() && AddToScope &&
-       !(D.isRedeclaration() && New->isInvalidDecl())) {
+  if (New->getDeclName() && AddToScope && !(D.isRedeclaration()
+      && New->isInvalidDecl() && !D.isFunctionDefinition())) {
     // Only make a locally-scoped extern declaration visible if it is the first
     // declaration of this entity. Qualified lookup for such an entity should
     // only find this declaration if there is no visible declaration of it.

test/Misc/ast-dump-invalid.cpp

 // CHECK-NEXT:         `-ImplicitCastExpr {{.*}} <col:12> 'int' <LValueToRValue>
 // CHECK-NEXT:           `-DeclRefExpr {{.*}} <col:12> 'int' lvalue ParmVar {{.*}} 'i' 'int'
 
+
+namespace TestInvalidFunctionDecl {
+struct Str {
+   double foo1(double, invalid_type);
+};
+double Str::foo1(double, invalid_type)
+{ return 45; }
+}
+// CHECK: NamespaceDecl {{.*}} <{{.*}}> {{.*}} TestInvalidFunctionDecl
+// CHECK-NEXT: |-CXXRecordDecl {{.*}} <line:46:1, line:48:1> line:46:8 struct Str definition
+// CHECK-NEXT: | |-CXXRecordDecl {{.*}} <col:1, col:8> col:8 implicit struct Str
+// CHECK-NEXT: | `-CXXMethodDecl {{.*}} <line:47:4, col:36> col:11 invalid foo1 'double (double, int)'
+// CHECK-NEXT: |   |-ParmVarDecl {{.*}} <col:16> col:22 'double'
+// CHECK-NEXT: |   `-ParmVarDecl {{.*}} <col:24, <invalid sloc>> col:36 invalid 'int'
+// CHECK-NEXT: `-CXXMethodDecl {{.*}} parent {{.*}} <line:49:1, line:50:14> line:49:13 invalid foo1 'double (double, int)'
+// CHECK-NEXT:   |-ParmVarDecl {{.*}} <col:18> col:24 'double'
+// CHECK-NEXT:   |-ParmVarDecl {{.*}} <col:26, <invalid sloc>> col:38 invalid 'int'
+// CHECK-NEXT:   `-CompoundStmt {{.*}} <line:50:1, col:14>
+// CHECK-NEXT:     `-ReturnStmt {{.*}} <col:3, col:10>
+// CHECK-NEXT:       `-ImplicitCastExpr {{.*}} <col:10> 'double' <IntegralToFloating>
+// CHECK-NEXT:         `-IntegerLiteral {{.*}} <col:10> 'int' 45