[PATCH] D21834: Implementing 'If statement with Initializer'

[Richard Smith via cfe-commits]

rsmith added a comment.

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


================
Comment at: include/clang/AST/Stmt.h:890
@@ -889,3 +889,3 @@
   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(),
----------------
`*` on the right, please (you can run clang-format over the patch to handle minor details like this).

================
Comment at: lib/CodeGen/CGStmt.cpp:565
@@ +564,3 @@
+  if (S.getInit())
+      EmitStmt(S.getInit());
+
----------------
Too much indentation here.

================
Comment at: lib/Sema/SemaStmt.cpp:512
@@ -511,3 @@
-  if (InitStmt)
-    Diag(InitStmt->getLocStart(), diag::err_init_stmt_not_supported);
-
----------------
Please also delete this diagnostic from include/clang/Basic/DiagnosticSemaKinds.td.

================
Comment at: test/CodeGenCXX/cxx1z-init-statement.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-macosx10.7.0 -emit-llvm -o - %s -w | FileCheck %s
+
----------------
Probably best to run this with -std=c++1z, but it's not a big deal.

================
Comment at: test/CodeGenCXX/cxx1z-init-statement.cpp:5
@@ +4,3 @@
+void f() {
+  // CHECK:      %1 = alloca i32, align 4
+  // CHECK-NEXT: store i32 5, i32* %1, align 4
----------------
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).

================
Comment at: test/PCH/cxx1z-init-statement.cpp:8-9
@@ +7,3 @@
+
+void g0(void) { f0(5); }
+int g1(int x) { return f1(x); }
----------------
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.

================
Comment at: test/SemaCXX/cxx1z-init-statement-warn-unused.cpp:6
@@ +5,3 @@
+    ;
+  if (int a, b = 2; a) // expected-warning {{uninitialized}} expected-note {{to silence}}
+    ;
----------------
Maybe add a test like this:

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

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


http://reviews.llvm.org/D21834