[C++17] Allow an empty expression in an if init statement

[Rakete1111]

Summary

This fixes PR35381 and an additional bug where clang didn't warn about the C++17 extension when having an expression in the init statement.

Thanks Nicolas Lesser for contributing the patch.

[erik.pilkington]

Hi, thanks for working on this!
Can you add tests to make sure that this also works with switch statements (which also have this bug), and not with while? Also, it makes it a lot easier to review these patches if you add context lines to the diff.
Thanks,
Erik

lib/Parse/ParseExprCXX.cpp

1750

We should only be doing this if InitStmt != nullptr, right? I think this would lead us to be too permissive with while statements, which don't have this feature. Also, it would be nice to emit a c++14-compat warning here, like below.

[Rakete1111]

Added a test for the switch statement and added full context to the diff.

[Rakete1111]

Addressed final review comments by adding the appropriate warnings that were missing previously.

[Rakete1111]

Moved the while test outside of the #ifdef region for better coverage :)

[Rakete1111]

Added more test coverage for compatibility warnings, and fixed a bug at the same time :).

[Rakete1111]

Rebased + friendly 2018 ping :)

[rsmith]

lib/Parse/ParseExprCXX.cpp

1762

Please create a NullStmt here (Actions.ActOnNullStmt), rather than producing an AST that's indistinguishable from one where the init statement was omitted.

1783–1784

Use WarnOnInit() here.

test/CXX/stmt.stmt/stmt.select/p3.cpp

70–71

I think error recovery thought that a ) was omitted before the ;, so it imagined we had:

while (Var + Var); Var--) {}

Then it thought that a ; was omitted before the ), so it imagined we had:

while (Var + Var); Var--; ) {}

Then it saw a statement beginning with a ), and decided to skip to the next statement, which would skip past the ), the {, the }, and whatever statement you put next in the function.

Maybe we should parse as if an init-statement were permitted on a while loop (and produce an error after the fact if there's one present). But that should be done in a separate patch if you're interested in working on it.

[Rakete1111]

Addressed review comments. I'll write up a patch in the next coming days then :)

[Rakete1111]

Rebase and ping :)

[rsmith]

Thanks, LGTM

[Rakete1111]

@rsmith Can you commit please?

lib/Parse/ParseExprCXX.cpp

if (Tok.is(tok::code_completion)) {
     Actions.CodeCompleteOrdinaryName(getCurScope(), Sema::PCC_Condition);
     cutOffParsing();
     return Sema::ConditionError();
   }
 
   ParsedAttributesWithRange attrs(AttrFactory);
   MaybeParseCXX11Attributes(attrs);
 
+  const auto WarnOnInit = [this, &CK] {
+    Diag(Tok.getLocation(), getLangOpts().CPlusPlus17
+                                ? diag::warn_cxx14_compat_init_statement
+                                : diag::ext_init_statement)
+        << (CK == Sema::ConditionKind::Switch);
+  };
+
   // Determine what kind of thing we have.
   switch (isCXXConditionDeclarationOrInitStatement(InitStmt)) {
   case ConditionOrInitStatement::Expression: {
     ProhibitAttributes(attrs);
 
+    // We can have an empty expression here.
+    //   if (; true);
+    if (InitStmt && Tok.is(tok::semi)) {
+      WarnOnInit();
+      SourceLocation SemiLoc = ConsumeToken();
+      *InitStmt = Actions.ActOnNullStmt(SemiLoc);
+      return ParseCXXCondition(nullptr, Loc, CK);
+    }
+
     // Parse the expression.
     ExprResult Expr = ParseExpression(); // expression
     if (Expr.isInvalid())
       return Sema::ConditionError();
 
     if (InitStmt && Tok.is(tok::semi)) {
+      WarnOnInit();
       *InitStmt = Actions.ActOnExprStmt(Expr.get());
       ConsumeToken();
       return ParseCXXCondition(nullptr, Loc, CK);
     }
 
     return Actions.ActOnCondition(getCurScope(), Loc, Expr.get(), CK);
   }
 
   case ConditionOrInitStatement::InitStmtDecl: {
-    Diag(Tok.getLocation(), getLangOpts().CPlusPlus17
-                                ? diag::warn_cxx14_compat_init_statement
-                                : diag::ext_init_statement)
-        << (CK == Sema::ConditionKind::Switch);
+    WarnOnInit();
     SourceLocation DeclStart = Tok.getLocation(), DeclEnd;
     DeclGroupPtrTy DG =
         ParseSimpleDeclaration(DeclaratorContext::InitStmtContext, DeclEnd,
                                attrs, /*RequireSemi=*/true);
     *InitStmt = Actions.ActOnDeclStmt(DG, DeclStart, DeclEnd);
     return ParseCXXCondition(nullptr, Loc, CK);
   }
 

test/CXX/stmt.stmt/stmt.select/p3.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++1z -Wc++14-compat -verify %s -DCPP17
 
 int f();
 
 void g() {
   if (int x = f()) { // expected-note 2{{previous definition}}
     int x; // expected-error{{redefinition of 'x'}}
   } else {
     int x; // expected-error{{redefinition of 'x'}}
   }
 }
 
-
 void h() {
   if (int x = f()) // expected-note 2{{previous definition}}
     int x; // expected-error{{redefinition of 'x'}}
   else
     int x; // expected-error{{redefinition of 'x'}}
 }
+
+void ifInitStatement() {
+  int Var = 0;
+
+  if (int I = 0; true) {}
+  if (Var + Var; true) {}
+  if (; true) {}
+#ifdef CPP17
+  // expected-warning@-4 {{if initialization statements are incompatible with C++ standards before C++17}}
+  // expected-warning@-4 {{if initialization statements are incompatible with C++ standards before C++17}}
+  // expected-warning@-4 {{if initialization statements are incompatible with C++ standards before C++17}}
+#else
+  // expected-warning@-8 {{'if' initialization statements are a C++17 extension}}
+  // expected-warning@-8 {{'if' initialization statements are a C++17 extension}}
+  // expected-warning@-8 {{'if' initialization statements are a C++17 extension}}
+#endif
+}
+
+void switchInitStatement() {
+  int Var = 0;
+
+  switch (int I = 0; Var) {}
+  switch (Var + Var; Var) {}
+  switch (; Var) {}
+#ifdef CPP17
+  // expected-warning@-4 {{switch initialization statements are incompatible with C++ standards before C++17}}
+  // expected-warning@-4 {{switch initialization statements are incompatible with C++ standards before C++17}}
+  // expected-warning@-4 {{switch initialization statements are incompatible with C++ standards before C++17}}
+#else
+  // expected-warning@-8 {{'switch' initialization statements are a C++17 extension}}
+  // expected-warning@-8 {{'switch' initialization statements are a C++17 extension}}
+  // expected-warning@-8 {{'switch' initialization statements are a C++17 extension}}
+#endif
+}
+
+// TODO: Better diagnostics for while init statements.
+void whileInitStatement() {
+  while (int I = 10; I--); // expected-error {{expected ')'}}
+  // expected-note@-1 {{to match this '('}}
+  // expected-error@-2 {{use of undeclared identifier 'I'}}
+
+  int Var = 10;
+  while (Var + Var; Var--) {} // expected-error {{expected ')'}}
+  // expected-note@-1 {{to match this '('}}
+  // expected-error@-2 {{expected ';' after expression}}
+  // expected-error@-3 {{expected expression}}
+  // expected-warning@-4 {{while loop has empty body}}
+  // expected-note@-5 {{put the semicolon on a separate line to silence this warning}}
+}
+
+// TODO: This is needed because clang can't seem to diagnose invalid syntax after the
+// last loop above. It would be nice to remove this.
+void whileInitStatement2() {
+  while (; false) {} // expected-error {{expected expression}}
+  // expected-warning@-1 {{expression result unused}}
+  // expected-error@-2 {{expected ';' after expression}}
+  // expected-error@-3 {{expected expression}}
+}