[cfe-commits] [PATCH][Review Request] Handle CXXForRangeStmt

[Richard Smith]

Hi Jim,

On Wed, October 5, 2011 10:54, Jim Goodnow II wrote:
> Okay, I see what you mean. I found several other multiple identical
> DeclRefs as well. Using -ast-view makes it really obvious. I decided to
> fix it in SemaStmt.cpp. Here's a new patch that is more complete. I wanted to
> also add a test using STL iterators, but I'm concerned that specifying
> -std=c++0x will bereak the  test on systems that don't have
> c++11 standard headers yet which generates compiler errors. At any rate, this
> patch does pass the test properly. Let me know what you think.

Functionally, the patch looks good to me. I have a concern about a similar
issue with TreeTransform (which also reuses AST nodes where possible), but it
looks like we don't currently use it in a way where it could produce the same
AST node twice within the same function.

Some small issues, then I think this is fine to check in:

Index: lib/Sema/SemaStmt.cpp
===================================================================
--- lib/Sema/SemaStmt.cpp        (revision 141126)
+++ lib/Sema/SemaStmt.cpp        (working copy)
@@ -1337,12 +1337,16 @@
   if (!BeginEndDecl.get() && !RangeVarType->isDependentType()) {
     SourceLocation RangeLoc = RangeVar->getLocation();

-    ExprResult RangeRef = BuildDeclRefExpr(RangeVar,
+    ExprResult BeginRangeRef = BuildDeclRefExpr(RangeVar,
                                            RangeVarType.getNonReferenceType(),
                                            VK_LValue, ColonLoc);

Please fix the indentation here and elsewhere. Preferably avoid the 80 column
concern by saving RangeVarType.getNonReferenceType() between the two calls.

-    if (RangeRef.isInvalid())
+    if (BeginRangeRef.isInvalid())
       return StmtError();

+    ExprResult EndRangeRef = BuildDeclRefExpr(RangeVar,
+                                           RangeVarType.getNonReferenceType(),
+                                           VK_LValue, ColonLoc);
+

An assert that this is not invalid would seem prudent (here and later).

     QualType AutoType = Context.getAutoDeductType();
     Expr *Range = RangeVar->getInit();
     if (!Range)
@@ -1368,8 +1372,8 @@
       //   the program is ill-formed;

       // begin-expr is __range.
-      BeginExpr = RangeRef;
-      if (FinishForRangeVarDecl(*this, BeginVar, RangeRef.get(), ColonLoc,
+      BeginExpr = BeginRangeRef;
+      if (FinishForRangeVarDecl(*this, BeginVar, BeginRangeRef.get(), ColonLoc,
                                 diag::err_for_range_iter_deduction_failure)) {
         NoteForRangeBeginEndFunction(*this, BeginExpr.get(), BEF_begin);
         return StmtError();
@@ -1391,7 +1395,7 @@
       }

       // end-expr is __range + __bound.
-      EndExpr = ActOnBinOp(S, ColonLoc, tok::plus, RangeRef.get(),
+      EndExpr = ActOnBinOp(S, ColonLoc, tok::plus, EndRangeRef.get(),
                            BoundExpr.get());
       if (EndExpr.isInvalid())
         return StmtError();
@@ -1431,14 +1435,14 @@
       }

       BeginExpr = BuildForRangeBeginEndCall(*this, S, ColonLoc, BeginVar,
-                                            BEF_begin, BeginNameInfo,
-                                            BeginMemberLookup, RangeRef.get());
+                                        BEF_begin, BeginNameInfo,
+                                        BeginMemberLookup, BeginRangeRef.get());
       if (BeginExpr.isInvalid())
         return StmtError();

       EndExpr = BuildForRangeBeginEndCall(*this, S, ColonLoc, EndVar,
                                           BEF_end, EndNameInfo,
-                                          EndMemberLookup, RangeRef.get());
+                                          EndMemberLookup, EndRangeRef.get());
       if (EndExpr.isInvalid())
         return StmtError();
     }
@@ -1477,6 +1481,8 @@
     }

     // Build and check ++__begin expression.
+    BeginRef = BuildDeclRefExpr(BeginVar, BeginType.getNonReferenceType(),
+                                           VK_LValue, ColonLoc);
     IncrExpr = ActOnUnaryOp(S, ColonLoc, tok::plusplus, BeginRef.get());
     IncrExpr = ActOnFinishFullExpr(IncrExpr.get());
     if (IncrExpr.isInvalid()) {
@@ -1485,6 +1491,8 @@
     }

     // Build and check *__begin  expression.
+    BeginRef = BuildDeclRefExpr(BeginVar, BeginType.getNonReferenceType(),
+                                           VK_LValue, ColonLoc);
     ExprResult DerefExpr = ActOnUnaryOp(S, ColonLoc, tok::star, BeginRef.get());
     if (DerefExpr.isInvalid()) {
       NoteForRangeBeginEndFunction(*this, BeginExpr.get(), BEF_begin);
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp        (revision 141126)
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp        (working copy)
@@ -453,7 +453,6 @@
     case Stmt::CXXBindTemporaryExprClass:
     case Stmt::CXXCatchStmtClass:
     case Stmt::CXXDependentScopeMemberExprClass:
-    case Stmt::CXXForRangeStmtClass:
     case Stmt::CXXPseudoDestructorExprClass:
     case Stmt::CXXTemporaryObjectExprClass:
     case Stmt::CXXThrowExprClass:
@@ -501,6 +500,7 @@
     case Stmt::CaseStmtClass:
     case Stmt::CompoundStmtClass:
     case Stmt::ContinueStmtClass:
+    case Stmt::CXXForRangeStmtClass:
     case Stmt::DefaultStmtClass:
     case Stmt::DoStmtClass:
     case Stmt::ForStmtClass:
Index: lib/StaticAnalyzer/Core/CoreEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/CoreEngine.cpp        (revision 141126)
+++ lib/StaticAnalyzer/Core/CoreEngine.cpp        (working copy)
@@ -17,6 +17,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "clang/Index/TranslationUnit.h"
 #include "clang/AST/Expr.h"
+#include "clang/AST/StmtCXX.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/ADT/DenseMap.h"
 using namespace clang;
@@ -349,6 +350,10 @@
         HandleBranch(cast<DoStmt>(Term)->getCond(), Term, B, Pred);
         return;

+      case Stmt::CXXForRangeStmtClass:
+        HandleBranch(cast<CXXForRangeStmt>(Term)->getCond(), Term, B, Pred );

Stray space before the close paren.

+        return;
+
       case Stmt::ForStmtClass:
         HandleBranch(cast<ForStmt>(Term)->getCond(), Term, B, Pred);
         return;