Implementing 'If statement with Initializer'

[AntonBikineev]

Summary

Here is an implementation of a feature 'if (init; condition)', proposed in p0305r0 and accepted at the last meeting in Oulu:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0305r0.html

[mclow.lists]

I think you should add "cfe-commits" to the subscribers.

[rsmith]

The proposal has changed since the pre-meeting mailing, and now covers switch as well as if. You also need to handle the expression-statement form of init-statement.

As it happens, I've also been working on this proposal, and the bit that you're getting wrong here (the parsing side) is the part that I've finished. I've committed that as r274169; if you rebase your Sema changes on top of that (and add support for switch), we can get this change committed to finish off the implementation.

We'll also need some tests before this can be committed.

Thank you!

lib/Parse/ParseStmt.cpp

1062–1075

We should try to only walk over the tokens once to identify whether this is a condition declaration, a simple-declaration of an init-statement, or an expression.

lib/Parse/ParseTentative.cpp

77–79

This is not correct; TryParseSimpleDeclaration usually stops long before it reaches the end of the simple-declaration, so this will give false negatives in lots of cases.

[AntonBikineev]

@rsmith,

This is not correct; TryParseSimpleDeclaration usually stops long before it reaches the end of the simple-declaration, so this will give false negatives in lots of cases.

Yes, I noticed that, that's why I changed use of TryParseSimpleDeclaration to something easier in last revision.

Thanks for reply, Richard. The revision you sent looks great, I should have updated my working copy before :)
Anyways, there are not too much of changes to Sema/AST stuff, I just applied them on top of yours, added the same for SwitchStmt. I'll update the test you put according to my changes tomorrow.

[AntonBikineev]

Test Parser/cxx1z-init-statement.cpp has been updated according to Sema changes

[rsmith]

There are a few missing pieces here:

1. Analysis/CFG.cpp needs to be taught to build a correct CFG for these. You can test this with an example like 'if (bool b; b)' which should give a -Wuninitialized warning.
2. AST/ExprConstant.cpp needs to be taught to perform constant evaluation for these properly.
3. More test coverage. In addition for test cases for the above two cases, please also add some tests to test/CodeGenCXX to test that we emit correct code for the expression and declaration form of these constructs, for both if and switch, and add a test that checks that the extra statement round-trips through an AST file properly (see test/PCH for some examples).

[AntonBikineev]

@rsmith,
Thanks for the comments, I've addressed them (except for tests, they are to be added asap). I'm especially not sure about changes in CFG.cpp, could you verify please?

[AntonBikineev]

@rsmith,
I've added some tests for c++1z init statement. Please let me know if there is anything else I should add or change.

[rsmith]

This looks really good. Some minor comments then this is ready to commit.

include/clang/AST/Stmt.h

890

* on the right, please (you can run clang-format over the patch to handle minor details like this).

lib/CodeGen/CGStmt.cpp

565

Too much indentation here.

lib/Sema/SemaStmt.cpp

512

Please also delete this diagnostic from include/clang/Basic/DiagnosticSemaKinds.td.

test/CodeGenCXX/cxx1z-init-statement.cpp

1 ↗

(On Diff #63056)

Probably best to run this with -std=c++1z, but it's not a big deal.

5 ↗

(On Diff #63056)

Please use a placeholder for the SSA variables here:

%[[A:.*]] = alloca i32
store i32 5, i32* %[[A]]

... so that this test doesn't depend on whether we generate names for these values or not (debug and release builds of Clang differ here in some cases, and it's not a salient part of the test).

test/PCH/cxx1z-init-statement.cpp

8–9 ↗

(On Diff #63056)

Maybe change this test to use constexpr and static_assert so that we can test that the behavior is correct, not just that it doesn't crash.

test/SemaCXX/cxx1z-init-statement-warn-unused.cpp

6 ↗

(On Diff #63056)

Maybe add a test like this:

int a;
if (a = 0; a) {}

... to make sure we actually visit the init-statement.

[AntonBikineev]

@rsmith,
Richard, again, thank you for guiding. I've addressed your comments.

[AntonBikineev]

removed a leftover from parser/cxx1z-init-stmt.cpp test

[AntonBikineev]

Moved stars to the right side of declarations

[rsmith]

Thank you again! I've fixed some over-long lines and committed this as r275350.

include/clang/AST/Stmt.h

 /// IfStmt - This represents an if/then/else.
 ///
 class IfStmt : public Stmt {
-  enum { VAR, COND, THEN, ELSE, END_EXPR };
+  enum { INIT, VAR, COND, THEN, ELSE, END_EXPR };
   Stmt* SubExprs[END_EXPR];
 
   SourceLocation IfLoc;

 
 public:
   IfStmt(const ASTContext &C, SourceLocation IL,
-         bool IsConstexpr, VarDecl *var, Expr *cond,
+         bool IsConstexpr, Stmt *init, VarDecl *var, Expr *cond,
          Stmt *then, SourceLocation EL = SourceLocation(),
          Stmt *elsev = nullptr);
 

   VarDecl *getConditionVariable() const;
   void setConditionVariable(const ASTContext &C, VarDecl *V);
 
+  Stmt *getInit() { return SubExprs[INIT]; }
+  const Stmt *getInit() const { return SubExprs[INIT]; }
+  void setInit(Stmt *S) { SubExprs[INIT] = S; }
+
   /// If this IfStmt has a condition variable, return the faux DeclStmt
   /// associated with the creation of that condition variable.
   const DeclStmt *getConditionVariableDeclStmt() const {

include/clang/Parse/Parser.h

   StmtResult ParseCompoundStatementBody(bool isStmtExpr = false);
   bool ParseParenExprOrCondition(Sema::ConditionResult &CondResult,
                                  SourceLocation Loc,
-                                 Sema::ConditionKind CK);
+                                 Sema::ConditionKind CK,
+                                 Stmt** Init = nullptr);
   StmtResult ParseIfStatement(SourceLocation *TrailingElseLoc);
   StmtResult ParseSwitchStatement(SourceLocation *TrailingElseLoc);
   StmtResult ParseWhileStatement(SourceLocation *TrailingElseLoc);

     return isDeclarationSpecifier(true);
   }
 
+  /// isForInitDeclarationWithSemi - is the same as isForInitDeclaration
+  /// but makes sure that there is a semicolon at the of the declaration
+  /// (needed for C++1z 'if statement with init')
+  bool isForInitDeclarationWithSemi();
+
   /// \brief Determine whether this is a C++1z for-range-identifier.
   bool isForRangeIdentifier();
 

include/clang/Sema/Sema.h

 
   class ConditionResult;
   StmtResult ActOnIfStmt(SourceLocation IfLoc, bool IsConstexpr,
-                         ConditionResult Cond, Stmt *ThenVal,
+                         ConditionResult Cond, Stmt *InitVal, Stmt *ThenVal,
                          SourceLocation ElseLoc, Stmt *ElseVal);
   StmtResult BuildIfStmt(SourceLocation IfLoc, bool IsConstexpr,
-                         ConditionResult Cond, Stmt *ThenVal,
+                         ConditionResult Cond, Stmt *InitVal, Stmt *ThenVal,
                          SourceLocation ElseLoc, Stmt *ElseVal);
   StmtResult ActOnStartOfSwitchStmt(SourceLocation SwitchLoc,
                                     ConditionResult Cond);

   enum class ConditionKind {
     Boolean,     ///< A boolean condition, from 'if', 'while', 'for', or 'do'.
     ConstexprIf, ///< A constant boolean condition from 'if constexpr'.
-    Switch       ///< An integral condition for a 'switch' statement.
+    Switch,      ///< An integral condition for a 'switch' statement.
+    IfWithInit   ///< A for-init-statement from C++17's 'if statement with initializer'
   };
 
   ConditionResult ActOnCondition(Scope *S, SourceLocation Loc,

lib/AST/ASTImporter.cpp

 Stmt *ASTNodeImporter::VisitIfStmt(IfStmt *S) {
   SourceLocation ToIfLoc = Importer.Import(S->getIfLoc());
   VarDecl *ToConditionVariable = nullptr;
+  Stmt *ToInit = Importer.Import(S->getInit());
+  if (!ToInit && S->getInit())
+    return nullptr;
   if (VarDecl *FromConditionVariable = S->getConditionVariable()) {
     ToConditionVariable =
       dyn_cast_or_null<VarDecl>(Importer.Import(FromConditionVariable));

     return nullptr;
   return new (Importer.getToContext()) IfStmt(Importer.getToContext(),
                                               ToIfLoc, S->isConstexpr(),
+                                              ToInit,
                                               ToConditionVariable,
                                               ToCondition, ToThenStmt,
                                               ToElseLoc, ToElseStmt);

lib/AST/Stmt.cpp

 }
 
 IfStmt::IfStmt(const ASTContext &C, SourceLocation IL, bool IsConstexpr,
-               VarDecl *var, Expr *cond, Stmt *then, SourceLocation EL,
+               Stmt *init, VarDecl *var, Expr *cond, Stmt *then, SourceLocation EL,
                Stmt *elsev)
     : Stmt(IfStmtClass), IfLoc(IL), ElseLoc(EL) {
   setConstexpr(IsConstexpr);
   setConditionVariable(C, var);
+  SubExprs[INIT] = init;
   SubExprs[COND] = cond;
   SubExprs[THEN] = then;
   SubExprs[ELSE] = elsev;

lib/Analysis/BodyFarm.cpp

                                            SourceLocation());
   
   // (5) Create the 'if' statement.
-  IfStmt *If = new (C) IfStmt(C, SourceLocation(), false, nullptr, UO, CS);
+  IfStmt *If = new (C) IfStmt(C, SourceLocation(), false, nullptr, nullptr, UO, CS);
   return If;
 }
 

   
   /// Construct the If.
   Stmt *If =
-    new (C) IfStmt(C, SourceLocation(), false, nullptr, Comparison, Body,
+    new (C) IfStmt(C, SourceLocation(), false, nullptr, nullptr, Comparison, Body,
                    SourceLocation(), Else);
 
   return If;  

lib/CodeGen/CGStmt.cpp

   // unequal to 0.  The condition must be a scalar type.
   LexicalScope ConditionScope(*this, S.getCond()->getSourceRange());
 
+  if (S.getInit())
+    EmitStmt(S.getInit());
+
   if (S.getConditionVariable())
     EmitAutoVarDecl(*S.getConditionVariable());
 

lib/Parse/ParseStmt.cpp

 /// errors in the condition.
 bool Parser::ParseParenExprOrCondition(Sema::ConditionResult &Cond,
                                        SourceLocation Loc,
-                                       Sema::ConditionKind CK) {
+                                       Sema::ConditionKind CK,
+                                       Stmt** Init) {
   BalancedDelimiterTracker T(*this, tok::l_paren);
   T.consumeOpen();
 
+  if (CK == Sema::ConditionKind::IfWithInit &&
+      isForInitDeclarationWithSemi()) {  // if (int X = 4;
+    // Parse declaration, which eats the ';'.
+    ParsedAttributesWithRange attrs(AttrFactory);
+    MaybeParseCXX11Attributes(attrs);
+
+    SourceLocation DeclStart = Tok.getLocation(), DeclEnd;
+    DeclGroupPtrTy DG = ParseSimpleDeclaration(
+      Declarator::ForContext, DeclEnd, attrs, false, nullptr);
+    StmtResult InitStmt = Actions.ActOnDeclStmt(DG, DeclStart, Tok.getLocation());
+    if (Init) *Init = InitStmt.get();
+
+    ConsumeToken(); // Consume semi
+  }
+
   if (getLangOpts().CPlusPlus)
     Cond = ParseCXXCondition(Loc, CK);
   else {

 
   // Parse the condition.
   Sema::ConditionResult Cond;
+  Stmt* Init = nullptr;
   if (ParseParenExprOrCondition(Cond, IfLoc,
                                 IsConstexpr ? Sema::ConditionKind::ConstexprIf
-                                            : Sema::ConditionKind::Boolean))
+                                            : getLangOpts().CPlusPlus1z
+                                                   ? Sema::ConditionKind::IfWithInit
+                                                   : Sema::ConditionKind::Boolean,
+                                &Init))
+
     return StmtError();
 
   llvm::Optional<bool> ConstexprCondition;

   if (ElseStmt.isInvalid())
     ElseStmt = Actions.ActOnNullStmt(ElseStmtLoc);
 
-  return Actions.ActOnIfStmt(IfLoc, IsConstexpr, Cond, ThenStmt.get(), ElseLoc,
-                             ElseStmt.get());
+  return Actions.ActOnIfStmt(IfLoc, IsConstexpr, Cond, Init,
+                             ThenStmt.get(), ElseLoc, ElseStmt.get());
 }
 
 /// ParseSwitchStatement

lib/Parse/ParseTentative.cpp

 
 #include "clang/Parse/Parser.h"
 #include "clang/Parse/ParseDiagnostic.h"
+#include "RAIIObjectsForParser.h"
 #include "clang/Sema/ParsedTemplate.h"
 using namespace clang;
 

   }
 }
 
+/// isForInitDeclarationWithSemi - is the same as isForInitDeclaration
+/// but makes sure that there is a semicolon at the of the declaration
+/// (needed for C++1z 'if statement with init')
+bool Parser::isForInitDeclarationWithSemi() {
+  if (!isForInitDeclaration())
+    return false;
+
+  TentativeParsingAction PA(*this);
+  bool NotFound = SkipUntil(tok::r_paren, StopAtSemi);
+  PA.Revert();
+
+  return !NotFound;
+}
+
 /// isCXXSimpleDeclaration - C++-specialized function that disambiguates
 /// between a simple-declaration or an expression-statement.
 /// If during the disambiguation process a parsing error is encountered,

lib/Sema/SemaExpr.cpp

   ExprResult Cond;
   switch (CK) {
   case ConditionKind::Boolean:
+  case ConditionKind::IfWithInit:
     Cond = CheckBooleanCondition(Loc, SubExpr);
     break;
 

lib/Sema/SemaExprCXX.cpp

 
   switch (CK) {
   case ConditionKind::Boolean:
+  case ConditionKind::IfWithInit:
     return CheckBooleanCondition(StmtLoc, Condition.get());
 
   case ConditionKind::ConstexprIf:

lib/Sema/SemaStmt.cpp

 
 StmtResult
 Sema::ActOnIfStmt(SourceLocation IfLoc, bool IsConstexpr, ConditionResult Cond,
-                  Stmt *thenStmt, SourceLocation ElseLoc,
+                  Stmt *initStmt, Stmt *thenStmt, SourceLocation ElseLoc,
                   Stmt *elseStmt) {
   if (Cond.isInvalid())
     Cond = ConditionResult(

     DiagnoseEmptyStmtBody(CondExpr->getLocEnd(), thenStmt,
                           diag::warn_empty_if_body);
 
-  return BuildIfStmt(IfLoc, IsConstexpr, Cond, thenStmt, ElseLoc, elseStmt);
+  return BuildIfStmt(IfLoc, IsConstexpr, Cond, initStmt, thenStmt, ElseLoc, elseStmt);
 }
 
 StmtResult Sema::BuildIfStmt(SourceLocation IfLoc, bool IsConstexpr,
-                             ConditionResult Cond, Stmt *thenStmt,
+                             ConditionResult Cond, Stmt *initStmt, Stmt *thenStmt,
                              SourceLocation ElseLoc, Stmt *elseStmt) {
   if (Cond.isInvalid())
     return StmtError();

   DiagnoseUnusedExprResult(thenStmt);
   DiagnoseUnusedExprResult(elseStmt);
 
-  return new (Context) IfStmt(Context, IfLoc, IsConstexpr, Cond.get().first,
+  return new (Context) IfStmt(Context, IfLoc, IsConstexpr, initStmt, Cond.get().first,
                               Cond.get().second, thenStmt, ElseLoc, elseStmt);
 }
 

lib/Sema/TreeTransform.h

   /// By default, performs semantic analysis to build the new statement.
   /// Subclasses may override this routine to provide different behavior.
   StmtResult RebuildIfStmt(SourceLocation IfLoc, bool IsConstexpr,
-                           Sema::ConditionResult Cond, Stmt *Then,
+                           Sema::ConditionResult Cond, Stmt *Init, Stmt *Then,
                            SourceLocation ElseLoc, Stmt *Else) {
-    return getSema().ActOnIfStmt(IfLoc, IsConstexpr, Cond, Then, ElseLoc, Else);
+    return getSema().ActOnIfStmt(IfLoc, IsConstexpr, Cond, Init, Then, ElseLoc, Else);
   }
 
   /// \brief Start building a new switch statement.

 template<typename Derived>
 StmtResult
 TreeTransform<Derived>::TransformIfStmt(IfStmt *S) {
+  // Transform the initialization statement
+  StmtResult Init = getDerived().TransformStmt(S->getInit());
+  if (Init.isInvalid())
+    return StmtError();
+
   // Transform the condition
   Sema::ConditionResult Cond = getDerived().TransformCondition(
       S->getIfLoc(), S->getConditionVariable(), S->getCond(),

   }
 
   if (!getDerived().AlwaysRebuild() &&
+      Init.get() == S->getInit() &&
       Cond.get() == std::make_pair(S->getConditionVariable(), S->getCond()) &&
       Then.get() == S->getThen() &&
       Else.get() == S->getElse())

     return S;
 
   return getDerived().RebuildIfStmt(S->getIfLoc(), S->isConstexpr(), Cond,
-                                    Then.get(), S->getElseLoc(), Else.get());
+                                    Init.get(), Then.get(), S->getElseLoc(), Else.get());
 }
 
 template<typename Derived>

lib/Serialization/ASTReaderStmt.cpp

 void ASTStmtReader::VisitIfStmt(IfStmt *S) {
   VisitStmt(S);
   S->setConstexpr(Record[Idx++]);
+  S->setInit(Reader.ReadSubStmt());
   S->setConditionVariable(Reader.getContext(),
                           ReadDeclAs<VarDecl>(Record, Idx));
   S->setCond(Reader.ReadSubExpr());

lib/Serialization/ASTWriterStmt.cpp

 void ASTStmtWriter::VisitIfStmt(IfStmt *S) {
   VisitStmt(S);
   Record.push_back(S->isConstexpr());
+  Record.AddStmt(S->getInit());
   Record.AddDeclRef(S->getConditionVariable());
   Record.AddStmt(S->getCond());
   Record.AddStmt(S->getThen());