From 276880ae8314be5598148a3b3bb7237d046406f8 Mon Sep 17 00:00:00 2001 From: Geoff Romer Date: Mon, 28 Jun 2021 15:57:54 -0700 Subject: [PATCH] Migrate `Tuple` and `FieldInitializer` to value semantics (#605) * Store tuple elements by value. * Update FieldInitializer to use IndirectValue. --- executable_semantics/ast/expression.cpp | 21 ++++--------- executable_semantics/ast/expression.h | 7 ++--- .../interpreter/interpreter.cpp | 24 +++++++-------- .../interpreter/typecheck.cpp | 27 +++++++++-------- .../syntax/paren_contents.cpp | 5 ++-- .../syntax/paren_contents_test.cpp | 30 +++++++++---------- executable_semantics/syntax/parser.ypp | 8 ++--- 7 files changed, 55 insertions(+), 67 deletions(-) diff --git a/executable_semantics/ast/expression.cpp b/executable_semantics/ast/expression.cpp index 27f288a2a451..c0b7f48a0df5 100644 --- a/executable_semantics/ast/expression.cpp +++ b/executable_semantics/ast/expression.cpp @@ -165,13 +165,13 @@ auto Expression::MakeGetField(int line_num, const Expression* exp, return e; } -auto Expression::MakeTuple(int line_num, std::vector* args) +auto Expression::MakeTuple(int line_num, std::vector args) -> const Expression* { auto* e = new Expression(); e->line_num = line_num; int i = 0; bool seen_named_member = false; - for (auto& arg : *args) { + for (auto& arg : args) { if (arg.name == "") { if (seen_named_member) { std::cerr << line_num @@ -189,17 +189,6 @@ auto Expression::MakeTuple(int line_num, std::vector* args) return e; } -// Create an AST node for an empty tuple. -// TODO(geoffromer): remove this and rewrite its callers to use -// `MakeTuple(line_num, {})`, once that works. -auto Expression::MakeUnit(int line_num) -> const Expression* { - auto* unit = new Expression(); - unit->line_num = line_num; - auto* args = new std::vector(); - unit->value = Tuple({.fields = args}); - return unit; -} - auto Expression::MakeIndex(int line_num, const Expression* exp, const Expression* i) -> const Expression* { auto* e = new Expression(); @@ -237,14 +226,14 @@ static void PrintOp(Operator op) { } } -static void PrintFields(std::vector* fields) { +static void PrintFields(const std::vector& fields) { int i = 0; - for (auto iter = fields->begin(); iter != fields->end(); ++iter, ++i) { + for (auto iter = fields.begin(); iter != fields.end(); ++iter, ++i) { if (i != 0) { std::cout << ", "; } std::cout << iter->name << " = "; - PrintExp(iter->expression); + PrintExp(iter->expression.GetPointer()); } } diff --git a/executable_semantics/ast/expression.h b/executable_semantics/ast/expression.h index 2ba68a3436af..9802941b91f1 100644 --- a/executable_semantics/ast/expression.h +++ b/executable_semantics/ast/expression.h @@ -21,7 +21,7 @@ struct FieldInitializer { std::string name; // The expression that initializes the field. - const Expression* expression; + IndirectValue expression; }; enum class ExpressionKind { @@ -92,7 +92,7 @@ struct BoolLiteral { struct Tuple { static constexpr ExpressionKind Kind = ExpressionKind::Tuple; - std::vector* fields; + std::vector fields; }; struct PrimitiveOperator { @@ -152,9 +152,8 @@ struct Expression { const Expression* arg) -> const Expression*; static auto MakeGetField(int line_num, const Expression* exp, std::string field) -> const Expression*; - static auto MakeTuple(int line_num, std::vector* args) + static auto MakeTuple(int line_num, std::vector args) -> const Expression*; - static auto MakeUnit(int line_num) -> const Expression*; static auto MakeIndex(int line_num, const Expression* exp, const Expression* i) -> const Expression*; static auto MakeTypeType(int line_num) -> const Expression*; diff --git a/executable_semantics/interpreter/interpreter.cpp b/executable_semantics/interpreter/interpreter.cpp index a7e2fb809da2..3c287f4dd5fe 100644 --- a/executable_semantics/interpreter/interpreter.cpp +++ b/executable_semantics/interpreter/interpreter.cpp @@ -427,7 +427,7 @@ void CreateTuple(Frame* frame, Action* act, const Expression* /*exp*/) { // { { (v1,...,vn) :: C, E, F} :: S, H} // -> { { `(v1,...,vn) :: C, E, F} :: S, H} auto elements = new std::vector(); - auto f = act->u.exp->GetTuple().fields->begin(); + auto f = act->u.exp->GetTuple().fields.begin(); for (auto i = act->results.begin(); i != act->results.end(); ++i, ++f) { Address a = state->heap.AllocateValue(*i); // copy? elements->push_back({.name = f->name, .address = a}); @@ -653,7 +653,7 @@ void StepLvalue() { case ExpressionKind::Tuple: { // { {(f1=e1,...) :: C, E, F} :: S, H} // -> { {e1 :: (f1=[],...) :: C, E, F} :: S, H} - const Expression* e1 = (*exp->GetTuple().fields)[0].expression; + const Expression* e1 = exp->GetTuple().fields[0].expression.GetPointer(); frame->todo.Push(MakeLvalAct(e1)); act->pos++; break; @@ -701,10 +701,11 @@ void StepExp() { break; } case ExpressionKind::Tuple: { - if (exp->GetTuple().fields->size() > 0) { + if (exp->GetTuple().fields.size() > 0) { // { {(f1=e1,...) :: C, E, F} :: S, H} // -> { {e1 :: (f1=[],...) :: C, E, F} :: S, H} - const Expression* e1 = (*exp->GetTuple().fields)[0].expression; + const Expression* e1 = + exp->GetTuple().fields[0].expression.GetPointer(); frame->todo.Push(MakeExpAct(e1)); act->pos++; } else { @@ -946,7 +947,7 @@ void StepStmt() { scopes.Push(scope); Stack todo; todo.Push(MakeStmtAct(Statement::MakeReturn( - stmt->line_num, Expression::MakeUnit(stmt->line_num)))); + stmt->line_num, Expression::MakeTuple(stmt->line_num, {})))); todo.Push(MakeStmtAct(stmt->GetContinuation().body)); Frame* continuation_frame = new Frame("__continuation", scopes, todo); Address continuation_address = state->heap.AllocateValue( @@ -1111,13 +1112,13 @@ void HandleValue() { break; } case ExpressionKind::Tuple: { - if (act->pos != static_cast(exp->GetTuple().fields->size())) { + if (act->pos != static_cast(exp->GetTuple().fields.size())) { // { { vk :: (f1=v1,..., fk=[],fk+1=ek+1,...) :: C, E, F} :: S, // H} // -> { { ek+1 :: (f1=v1,..., fk=vk, fk+1=[],...) :: C, E, F} :: S, // H} const Expression* elt = - (*exp->GetTuple().fields)[act->pos].expression; + exp->GetTuple().fields[act->pos].expression.GetPointer(); frame->todo.Pop(1); frame->todo.Push(MakeLvalAct(elt)); } else { @@ -1144,13 +1145,13 @@ void HandleValue() { break; } case ExpressionKind::Tuple: { - if (act->pos != static_cast(exp->GetTuple().fields->size())) { + if (act->pos != static_cast(exp->GetTuple().fields.size())) { // { { vk :: (f1=v1,..., fk=[],fk+1=ek+1,...) :: C, E, F} :: S, // H} // -> { { ek+1 :: (f1=v1,..., fk=vk, fk+1=[],...) :: C, E, F} :: S, // H} const Expression* elt = - (*exp->GetTuple().fields)[act->pos].expression; + exp->GetTuple().fields[act->pos].expression.GetPointer(); frame->todo.Pop(1); frame->todo.Push(MakeExpAct(elt)); } else { @@ -1424,7 +1425,7 @@ void HandleValue() { // Push an expression statement action to ignore the result // value from the continuation. Action* ignore_result = MakeStmtAct(Statement::MakeExpStmt( - stmt->line_num, Expression::MakeUnit(stmt->line_num))); + stmt->line_num, Expression::MakeTuple(stmt->line_num, {}))); ignore_result->pos = 0; frame->todo.Push(ignore_result); // Push the continuation onto the current stack. @@ -1497,8 +1498,7 @@ auto InterpProgram(std::list* fs) -> int { } InitGlobals(fs); - const Expression* arg = - Expression::MakeTuple(0, new std::vector()); + const Expression* arg = Expression::MakeTuple(0, {}); const Expression* call_main = Expression::MakeCall(0, Expression::MakeVar(0, "main"), arg); auto todo = Stack(MakeExpAct(call_main)); diff --git a/executable_semantics/interpreter/typecheck.cpp b/executable_semantics/interpreter/typecheck.cpp index 60d6dea6ff8d..f6ca4ce6df51 100644 --- a/executable_semantics/interpreter/typecheck.cpp +++ b/executable_semantics/interpreter/typecheck.cpp @@ -70,12 +70,12 @@ auto ReifyType(const Value* t, int line_num) -> const Expression* { 0, ReifyType(t->GetFunctionType().param, line_num), ReifyType(t->GetFunctionType().ret, line_num)); case ValKind::TupleV: { - auto args = new std::vector(); + std::vector args; for (const TupleElement& field : *t->GetTuple().elements) { - args->push_back( + args.push_back( {.name = field.name, - .expression = ReifyType(state->heap.Read(field.address, line_num), - line_num)}); + .expression = *ReifyType(state->heap.Read(field.address, line_num), + line_num)}); } return Expression::MakeTuple(0, args); } @@ -190,7 +190,7 @@ auto TypeCheckExp(const Expression* e, TypeEnv types, Env values, } } case ExpressionKind::Tuple: { - auto new_args = new std::vector(); + std::vector new_args; auto arg_types = new std::vector(); auto new_types = types; if (expected && expected->tag != ValKind::TupleV) { @@ -198,7 +198,7 @@ auto TypeCheckExp(const Expression* e, TypeEnv types, Env values, << std::endl; exit(-1); } - if (expected && e->GetTuple().fields->size() != + if (expected && e->GetTuple().fields.size() != expected->GetTuple().elements->size()) { std::cerr << e->line_num << ": compilation error, tuples of different length" @@ -206,8 +206,8 @@ auto TypeCheckExp(const Expression* e, TypeEnv types, Env values, exit(-1); } int i = 0; - for (auto arg = e->GetTuple().fields->begin(); - arg != e->GetTuple().fields->end(); ++arg, ++i) { + for (auto arg = e->GetTuple().fields.begin(); + arg != e->GetTuple().fields.end(); ++arg, ++i) { const Value* arg_expected = nullptr; if (expected && expected->tag == ValKind::TupleV) { if ((*expected->GetTuple().elements)[i].name != arg->name) { @@ -220,10 +220,10 @@ auto TypeCheckExp(const Expression* e, TypeEnv types, Env values, arg_expected = state->heap.Read( (*expected->GetTuple().elements)[i].address, e->line_num); } - auto arg_res = TypeCheckExp(arg->expression, new_types, values, - arg_expected, context); + auto arg_res = TypeCheckExp(arg->expression.GetPointer(), new_types, + values, arg_expected, context); new_types = arg_res.types; - new_args->push_back({.name = arg->name, .expression = arg_res.exp}); + new_args.push_back({.name = arg->name, .expression = *arg_res.exp}); arg_types->push_back( {.name = arg->name, .address = state->heap.AllocateValue(arg_res.type)}); @@ -586,7 +586,8 @@ auto CheckOrEnsureReturn(const Statement* stmt, bool void_return, int line_num) -> const Statement* { if (!stmt) { if (void_return) { - return Statement::MakeReturn(line_num, Expression::MakeUnit(line_num)); + return Statement::MakeReturn(line_num, + Expression::MakeTuple(line_num, {})); } else { std::cerr << "control-flow reaches end of non-void function without a return" @@ -643,7 +644,7 @@ auto CheckOrEnsureReturn(const Statement* stmt, bool void_return, int line_num) return Statement::MakeSeq( stmt->line_num, stmt, Statement::MakeReturn(stmt->line_num, - Expression::MakeUnit(stmt->line_num))); + Expression::MakeTuple(stmt->line_num, {}))); } else { std::cerr << stmt->line_num diff --git a/executable_semantics/syntax/paren_contents.cpp b/executable_semantics/syntax/paren_contents.cpp index d4b4cbe3199b..d3efd619c015 100644 --- a/executable_semantics/syntax/paren_contents.cpp +++ b/executable_semantics/syntax/paren_contents.cpp @@ -9,15 +9,14 @@ namespace Carbon { const Expression* ParenContents::AsExpression(int line_number) const { if (fields_.size() == 1 && fields_.front().name == "" && has_trailing_comma_ == HasTrailingComma::No) { - return fields_.front().expression; + return new Expression(*fields_.front().expression); } else { return AsTuple(line_number); } } const Expression* ParenContents::AsTuple(int line_number) const { - return Expression::MakeTuple(line_number, - new std::vector(fields_)); + return Expression::MakeTuple(line_number, fields_); } } // namespace Carbon diff --git a/executable_semantics/syntax/paren_contents_test.cpp b/executable_semantics/syntax/paren_contents_test.cpp index 5075c41afbcc..68326bbc37df 100644 --- a/executable_semantics/syntax/paren_contents_test.cpp +++ b/executable_semantics/syntax/paren_contents_test.cpp @@ -14,7 +14,7 @@ TEST(ParenContentsTest, EmptyAsExpression) { const Expression* expression = contents.AsExpression(/*line_num=*/1); EXPECT_EQ(expression->line_num, 1); ASSERT_EQ(expression->tag(), ExpressionKind::Tuple); - EXPECT_EQ(expression->GetTuple().fields->size(), 0); + EXPECT_EQ(expression->GetTuple().fields.size(), 0); } TEST(ParenContentsTest, EmptyAsTuple) { @@ -22,7 +22,7 @@ TEST(ParenContentsTest, EmptyAsTuple) { const Expression* tuple = contents.AsTuple(/*line_num=*/1); EXPECT_EQ(tuple->line_num, 1); ASSERT_EQ(tuple->tag(), ExpressionKind::Tuple); - EXPECT_EQ(tuple->GetTuple().fields->size(), 0); + EXPECT_EQ(tuple->GetTuple().fields.size(), 0); } TEST(ParenContentsTest, UnaryNoCommaAsExpression) { @@ -33,7 +33,7 @@ TEST(ParenContentsTest, UnaryNoCommaAsExpression) { // ) // ``` ParenContents contents( - {{.expression = Expression::MakeInt(/*line_num=*/2, 42)}}, + {{.expression = *Expression::MakeInt(/*line_num=*/2, 42)}}, ParenContents::HasTrailingComma::No); const Expression* expression = contents.AsExpression(/*line_num=*/1); @@ -43,53 +43,53 @@ TEST(ParenContentsTest, UnaryNoCommaAsExpression) { TEST(ParenContentsTest, UnaryNoCommaAsTuple) { ParenContents contents( - {{.expression = Expression::MakeInt(/*line_num=*/2, 42)}}, + {{.expression = *Expression::MakeInt(/*line_num=*/2, 42)}}, ParenContents::HasTrailingComma::No); const Expression* tuple = contents.AsTuple(/*line_num=*/1); EXPECT_EQ(tuple->line_num, 1); ASSERT_EQ(tuple->tag(), ExpressionKind::Tuple); - std::vector fields = *tuple->GetTuple().fields; + std::vector fields = tuple->GetTuple().fields; ASSERT_EQ(fields.size(), 1); EXPECT_EQ(fields[0].expression->tag(), ExpressionKind::Integer); } TEST(ParenContentsTest, UnaryWithCommaAsExpression) { ParenContents contents( - {{.expression = Expression::MakeInt(/*line_num=*/2, 42)}}, + {{.expression = *Expression::MakeInt(/*line_num=*/2, 42)}}, ParenContents::HasTrailingComma::Yes); const Expression* expression = contents.AsExpression(/*line_num=*/1); EXPECT_EQ(expression->line_num, 1); ASSERT_EQ(expression->tag(), ExpressionKind::Tuple); - std::vector fields = *expression->GetTuple().fields; + std::vector fields = expression->GetTuple().fields; ASSERT_EQ(fields.size(), 1); EXPECT_EQ(fields[0].expression->tag(), ExpressionKind::Integer); } TEST(ParenContentsTest, UnaryWithCommaAsTuple) { ParenContents contents( - {{.expression = Expression::MakeInt(/*line_num=*/2, 42)}}, + {{.expression = *Expression::MakeInt(/*line_num=*/2, 42)}}, ParenContents::HasTrailingComma::Yes); const Expression* tuple = contents.AsTuple(/*line_num=*/1); EXPECT_EQ(tuple->line_num, 1); ASSERT_EQ(tuple->tag(), ExpressionKind::Tuple); - std::vector fields = *tuple->GetTuple().fields; + std::vector fields = tuple->GetTuple().fields; ASSERT_EQ(fields.size(), 1); EXPECT_EQ(fields[0].expression->tag(), ExpressionKind::Integer); } TEST(ParenContentsTest, BinaryAsExpression) { ParenContents contents( - {{.expression = Expression::MakeInt(/*line_num=*/2, 42)}, - {.expression = Expression::MakeInt(/*line_num=*/3, 42)}}, + {{.expression = *Expression::MakeInt(/*line_num=*/2, 42)}, + {.expression = *Expression::MakeInt(/*line_num=*/3, 42)}}, ParenContents::HasTrailingComma::Yes); const Expression* expression = contents.AsExpression(/*line_num=*/1); EXPECT_EQ(expression->line_num, 1); ASSERT_EQ(expression->tag(), ExpressionKind::Tuple); - std::vector fields = *expression->GetTuple().fields; + std::vector fields = expression->GetTuple().fields; ASSERT_EQ(fields.size(), 2); EXPECT_EQ(fields[0].expression->tag(), ExpressionKind::Integer); EXPECT_EQ(fields[1].expression->tag(), ExpressionKind::Integer); @@ -97,14 +97,14 @@ TEST(ParenContentsTest, BinaryAsExpression) { TEST(ParenContentsTest, BinaryAsTuple) { ParenContents contents( - {{.expression = Expression::MakeInt(/*line_num=*/2, 42)}, - {.expression = Expression::MakeInt(/*line_num=*/3, 42)}}, + {{.expression = *Expression::MakeInt(/*line_num=*/2, 42)}, + {.expression = *Expression::MakeInt(/*line_num=*/3, 42)}}, ParenContents::HasTrailingComma::Yes); const Expression* tuple = contents.AsTuple(/*line_num=*/1); EXPECT_EQ(tuple->line_num, 1); ASSERT_EQ(tuple->tag(), ExpressionKind::Tuple); - std::vector fields = *tuple->GetTuple().fields; + std::vector fields = tuple->GetTuple().fields; ASSERT_EQ(fields.size(), 2); EXPECT_EQ(fields[0].expression->tag(), ExpressionKind::Integer); EXPECT_EQ(fields[1].expression->tag(), ExpressionKind::Integer); diff --git a/executable_semantics/syntax/parser.ypp b/executable_semantics/syntax/parser.ypp index fc0d12e33690..d3b28d70e454 100644 --- a/executable_semantics/syntax/parser.ypp +++ b/executable_semantics/syntax/parser.ypp @@ -264,9 +264,9 @@ tuple: "(" paren_contents ")" ; field_initializer: pattern - { $$ = Carbon::FieldInitializer({"", $1}); } + { $$ = Carbon::FieldInitializer({"", *$1}); } | designator "=" pattern - { $$ = Carbon::FieldInitializer({$1, $3}); } + { $$ = Carbon::FieldInitializer({$1, *$3}); } ; paren_contents: // Empty @@ -350,7 +350,7 @@ statement_list: ; return_type: // Empty - { $$ = Carbon::Expression::MakeUnit(yylineno); } + { $$ = Carbon::Expression::MakeTuple(yylineno, {}); } | ARROW expression %prec FNARROW { $$ = $2; } ; @@ -385,7 +385,7 @@ alternative: | identifier { $$ = new std::pair( - $1, Carbon::Expression::MakeUnit(yylineno)); + $1, Carbon::Expression::MakeTuple(yylineno, {})); } ; alternative_list: