Re: [PATCH] D19327: Keep invalid function body as part of the AST

[Olivier Goffart via cfe-commits]

ogoffart abandoned this revision.
ogoffart added a comment.

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


http://reviews.llvm.org/D19327



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19327: Keep invalid function body as part of the AST

[Olivier Goffart via cfe-commits]

ogoffart added a comment.

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


http://reviews.llvm.org/D19327



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19327: Keep invalid function body as part of the AST

[Olivier Goffart via cfe-commits]

ogoffart added inline comments.


Comment at: lib/Sema/SemaDecl.cpp:5044-5045
@@ -5043,4 +5043,4 @@
   // 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

rsmith wrote:
> 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.
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?
  


http://reviews.llvm.org/D19327



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19327: Keep invalid function body as part of the AST

[Richard Smith via cfe-commits]

rsmith added inline comments.


Comment at: lib/Sema/SemaDecl.cpp:5044-5045
@@ -5043,4 +5043,4 @@
   // 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

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.


http://reviews.llvm.org/D19327



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19327: Keep invalid function body as part of the AST

[Olivier Goffart via cfe-commits]

ogoffart added a comment.

Ping?


http://reviews.llvm.org/D19327



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19327: Keep invalid function body as part of the AST

[Olivier Goffart via cfe-commits]

ogoffart created this revision.
ogoffart added reviewers: cfe-commits, rsmith.


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.

http://reviews.llvm.org/D19327

Files:
  lib/Sema/SemaDecl.cpp
  test/Misc/ast-dump-invalid.cpp

Index: test/Misc/ast-dump-invalid.cpp
===
--- test/Misc/ast-dump-invalid.cpp
+++ test/Misc/ast-dump-invalid.cpp
@@ -41,3 +41,24 @@
 // CHECK-NEXT: `-ImplicitCastExpr {{.*}}  'int' 

 // CHECK-NEXT:   `-DeclRefExpr {{.*}}  '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:8 struct 
Str definition
+// CHECK-NEXT: | |-CXXRecordDecl {{.*}}  col:8 implicit struct 
Str
+// CHECK-NEXT: | `-CXXMethodDecl {{.*}}  col:11 invalid 
foo1 'double (double, int)'
+// CHECK-NEXT: |   |-ParmVarDecl {{.*}}  col:22 'double'
+// CHECK-NEXT: |   `-ParmVarDecl {{.*}}  col:36 
invalid 'int'
+// CHECK-NEXT: `-CXXMethodDecl {{.*}} parent {{.*}}  
line:49:13 invalid foo1 'double (double, int)'
+// CHECK-NEXT:   |-ParmVarDecl {{.*}}  col:24 'double'
+// CHECK-NEXT:   |-ParmVarDecl {{.*}}  col:38 invalid 
'int'
+// CHECK-NEXT:   `-CompoundStmt {{.*}} 
+// CHECK-NEXT: `-ReturnStmt {{.*}} 
+// CHECK-NEXT:   `-ImplicitCastExpr {{.*}}  'double' 

+// CHECK-NEXT: `-IntegerLiteral {{.*}}  'int' 45
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -5041,8 +5041,8 @@
 
   // 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.


Index: test/Misc/ast-dump-invalid.cpp
===
--- test/Misc/ast-dump-invalid.cpp
+++ test/Misc/ast-dump-invalid.cpp
@@ -41,3 +41,24 @@
 // CHECK-NEXT: `-ImplicitCastExpr {{.*}}  'int' 
 // CHECK-NEXT:   `-DeclRefExpr {{.*}}  '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:8 struct Str definition
+// CHECK-NEXT: | |-CXXRecordDecl {{.*}}  col:8 implicit struct Str
+// CHECK-NEXT: | `-CXXMethodDecl {{.*}}  col:11 invalid foo1 'double (double, int)'
+// CHECK-NEXT: |   |-ParmVarDecl {{.*}}  col:22 'double'
+// CHECK-NEXT: |   `-ParmVarDecl {{.*}}  col:36 invalid 'int'
+// CHECK-NEXT: `-CXXMethodDecl {{.*}} parent {{.*}}  line:49:13 invalid foo1 'double (double, int)'
+// CHECK-NEXT:   |-ParmVarDecl {{.*}}  col:24 'double'
+// CHECK-NEXT:   |-ParmVarDecl {{.*}}  col:38 invalid 'int'
+// CHECK-NEXT:   `-CompoundStmt {{.*}} 
+// CHECK-NEXT: `-ReturnStmt {{.*}} 
+// CHECK-NEXT:   `-ImplicitCastExpr {{.*}}  'double' 
+// CHECK-NEXT: `-IntegerLiteral {{.*}}  'int' 45
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -5041,8 +5041,8 @@
 
   // 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.