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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
