Re: [PATCH] D13344: Keep the IfStmt node even if the condition is invalid

[Olivier Goffart via cfe-commits]

ogoffart abandoned this revision.
ogoffart marked an inline comment as done.
ogoffart added a comment.

commited as r249982. (I forgot to add the link to reviews.llvm.org in the 
commit message. I'll do it next time)


http://reviews.llvm.org/D13344



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

Re: [PATCH] D13344: Keep the IfStmt node even if the condition is invalid

[Richard Smith via cfe-commits]

rsmith added inline comments.


Comment at: lib/Sema/SemaStmt.cpp:505-506
@@ -512,1 +504,4 @@
+  } else {
+ConditionExpr = new (Context) OpaqueValueExpr(SourceLocation(),
+  Context.VoidTy, VK_RValue);
   }

Please add a comment here saying that we're creating this node for error 
recovery. Also, the type of the expression should be `BoolTy`, not `VoidTy`. 
Other than that, this looks fine to me.


http://reviews.llvm.org/D13344



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

Re: [PATCH] D13344: Keep the IfStmt node even if the condition is invalid

[Olivier Goffart via cfe-commits]

ogoffart added a comment.

Are the analysis run if there is an error?  I think the consumer should expect 
null condition anyway.

But i'll try to add an ErrorExpr anyway.


http://reviews.llvm.org/D13344



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

Re: [PATCH] D13344: Keep the IfStmt node even if the condition is invalid

[Richard Smith via cfe-commits]

rsmith added inline comments.


Comment at: lib/Sema/SemaStmt.cpp:508-509
@@ -513,6 +507,4 @@
 
-  DiagnoseUnusedExprResult(elseStmt);
-
   return new (Context) IfStmt(Context, IfLoc, ConditionVar, ConditionExpr,
   thenStmt, ElseLoc, elseStmt);
 }

This will create an `IfStmt` with no `ConditionExpr`. That is not a valid AST 
construct, and it's not reasonable to expect all the downstream consumers of 
the AST to be able to cope with it. It's not hard to find parts of the codebase 
that will crash when presented with this:

  lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:1133
  lib/Analysis/Consumed.cpp:1265
  lib/Sema/AnalysisBasedWarnings.cpp:740

... and so on.

As the comment above suggests, you could create some sort of placeholder 
expression node here instead (either use something like an `OpaqueValueExpr`, 
or add a new Expr subclass to represent an erroneous expression -- the latter 
would probably be useful in many cases where we hit parse errors but can still 
produce some useful information to tools).


http://reviews.llvm.org/D13344



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

Re: [PATCH] D13344: Keep the IfStmt node even if the condition is invalid

[Manuel Klimek via cfe-commits]

klimek added a reviewer: rsmith.
klimek added a comment.

+Richard, whom we need to validate AST changes to make sure they're benign.


http://reviews.llvm.org/D13344



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

[PATCH] D13344: Keep the IfStmt node even if the condition is invalid

[Olivier Goffart via cfe-commits]

ogoffart created this revision.
ogoffart added a reviewer: cfe-commits.

This is quite useful for IDE's or other tools which would like to recover as 
much as possible even if there are a few errors in the code, and discarding 
the full 'if' bodies is unfortunate.

http://reviews.llvm.org/D13344

Files:
  lib/Sema/SemaStmt.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
@@ -18,3 +18,26 @@
 // CHECK-NEXT: `-CXXUnresolvedConstructExpr {{.*}}  'T'
 // CHECK-NEXT:   |-DeclRefExpr {{.*}}  'T' lvalue ParmVar 
{{.*}} 'i' 'T'
 // CHECK-NEXT:   `-DeclRefExpr {{.*}}  'T' lvalue ParmVar 
{{.*}} 'j' 'T'
+
+
+namespace TestInvalidIf {
+int g(int i) {
+  if (invalid_condition)
+return 4;
+  else
+return i;
+}
+}
+// CHECK: NamespaceDecl {{.*}} <{{.*}}> {{.*}} TestInvalidIf
+// CHECK-NEXT: `-FunctionDecl
+// CHECK-NEXT:   |-ParmVarDecl
+// CHECK-NEXT:   `-CompoundStmt
+// CHECK-NEXT: `-IfStmt {{.*}} 
+// CHECK-NEXT:   |-<<>>
+// CHECK-NEXT:   |-<<>>
+// CHECK-NEXT:   |-ReturnStmt {{.*}} 
+// CHECK-NEXT:   | `-IntegerLiteral {{.*}}  'int' 4
+// CHECK-NEXT:   `-ReturnStmt {{.*}} 
+// CHECK-NEXT: `-ImplicitCastExpr {{.*}}  'int' 

+// CHECK-NEXT:   `-DeclRefExpr {{.*}}  'int' lvalue ParmVar 
{{.*}} 'i' 'int'
+
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -483,13 +483,6 @@
 Sema::ActOnIfStmt(SourceLocation IfLoc, FullExprArg CondVal, Decl *CondVar,
   Stmt *thenStmt, SourceLocation ElseLoc,
   Stmt *elseStmt) {
-  // If the condition was invalid, discard the if statement.  We could recover
-  // better by replacing it with a valid expr, but don't do that yet.
-  if (!CondVal.get() && !CondVar) {
-getCurFunction()->setHasDroppedStmt();
-return StmtError();
-  }
-
   ExprResult CondResult(CondVal.release());
 
   VarDecl *ConditionVar = nullptr;
@@ -501,18 +494,17 @@
   return StmtError();
   }
   Expr *ConditionExpr = CondResult.getAs();
-  if (!ConditionExpr)
-return StmtError();
+  if (ConditionExpr) {
+DiagnoseUnusedExprResult(thenStmt);
 
-  DiagnoseUnusedExprResult(thenStmt);
+if (!elseStmt) {
+  DiagnoseEmptyStmtBody(ConditionExpr->getLocEnd(), thenStmt,
+diag::warn_empty_if_body);
+}
 
-  if (!elseStmt) {
-DiagnoseEmptyStmtBody(ConditionExpr->getLocEnd(), thenStmt,
-  diag::warn_empty_if_body);
+DiagnoseUnusedExprResult(elseStmt);
   }
 
-  DiagnoseUnusedExprResult(elseStmt);
-
   return new (Context) IfStmt(Context, IfLoc, ConditionVar, ConditionExpr,
   thenStmt, ElseLoc, elseStmt);
 }


Index: test/Misc/ast-dump-invalid.cpp
===
--- test/Misc/ast-dump-invalid.cpp
+++ test/Misc/ast-dump-invalid.cpp
@@ -18,3 +18,26 @@
 // CHECK-NEXT: `-CXXUnresolvedConstructExpr {{.*}}  'T'
 // CHECK-NEXT:   |-DeclRefExpr {{.*}}  'T' lvalue ParmVar {{.*}} 'i' 'T'
 // CHECK-NEXT:   `-DeclRefExpr {{.*}}  'T' lvalue ParmVar {{.*}} 'j' 'T'
+
+
+namespace TestInvalidIf {
+int g(int i) {
+  if (invalid_condition)
+return 4;
+  else
+return i;
+}
+}
+// CHECK: NamespaceDecl {{.*}} <{{.*}}> {{.*}} TestInvalidIf
+// CHECK-NEXT: `-FunctionDecl
+// CHECK-NEXT:   |-ParmVarDecl
+// CHECK-NEXT:   `-CompoundStmt
+// CHECK-NEXT: `-IfStmt {{.*}} 
+// CHECK-NEXT:   |-<<>>
+// CHECK-NEXT:   |-<<>>
+// CHECK-NEXT:   |-ReturnStmt {{.*}} 
+// CHECK-NEXT:   | `-IntegerLiteral {{.*}}  'int' 4
+// CHECK-NEXT:   `-ReturnStmt {{.*}} 
+// CHECK-NEXT: `-ImplicitCastExpr {{.*}}  'int' 
+// CHECK-NEXT:   `-DeclRefExpr {{.*}}  'int' lvalue ParmVar {{.*}} 'i' 'int'
+
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -483,13 +483,6 @@
 Sema::ActOnIfStmt(SourceLocation IfLoc, FullExprArg CondVal, Decl *CondVar,
   Stmt *thenStmt, SourceLocation ElseLoc,
   Stmt *elseStmt) {
-  // If the condition was invalid, discard the if statement.  We could recover
-  // better by replacing it with a valid expr, but don't do that yet.
-  if (!CondVal.get() && !CondVar) {
-getCurFunction()->setHasDroppedStmt();
-return StmtError();
-  }
-
   ExprResult CondResult(CondVal.release());
 
   VarDecl *ConditionVar = nullptr;
@@ -501,18 +494,17 @@
   return StmtError();
   }
   Expr *ConditionExpr = CondResult.getAs();
-  if (!ConditionExpr)
-return StmtError();
+  if 

Re: [PATCH] D13344: Keep the IfStmt node even if the condition is invalid

[Milian Wolff via cfe-commits]

milianw added a comment.

Is there still an error reported for the invalid condition?

Anyhow, from my POV this is excellent and should fix a bunch of issues I've 
seen when using clang-c in KDevelop. I never got around to investigating it, 
but it always was related to conditionals. I'm pretty sure this patch fixes it 
then.

@ogoffart: For Manuel to see the difference, could you share the before/after 
screenshots of your code browser? That makes the impact of this patch pretty 
clear.


http://reviews.llvm.org/D13344



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