From ff319d311a2a1c65e1f2bd9b97c89b96b63417a1 Mon Sep 17 00:00:00 2001 From: Jon Meow <46229924+jonmeow@users.noreply.github.com> Date: Thu, 16 Sep 2021 13:05:25 -0700 Subject: [PATCH] Switch Ptr to a C++ pointer using the nonnull attribute (#831) The advantage is a C++ pointer is special, and this approach eliminates the Ptr class type that was causing problems in conversions. Attribute suggestion was courtesy of chandlerc. We're sticking with the Ptr name because it's shorter than Nonnull, and we're likely to keep this in lots of places. --- executable_semantics/ast/expression_test.cpp | 24 ++++++------- executable_semantics/ast/pattern_test.cpp | 16 ++++----- executable_semantics/common/ptr.h | 35 +------------------ .../interpreter/interpreter.cpp | 4 +-- .../interpreter/type_checker.cpp | 7 ++-- executable_semantics/main.cpp | 4 +-- 6 files changed, 28 insertions(+), 62 deletions(-) diff --git a/executable_semantics/ast/expression_test.cpp b/executable_semantics/ast/expression_test.cpp index aca62b65f4b9..78eea43d9090 100644 --- a/executable_semantics/ast/expression_test.cpp +++ b/executable_semantics/ast/expression_test.cpp @@ -39,7 +39,7 @@ TEST_F(ExpressionTest, EmptyAsExpression) { ParenContents contents = {.elements = {}, .has_trailing_comma = false}; Ptr expression = - ExpressionFromParenContents(PtrTo(arena), FakeSourceLoc(1), contents); + ExpressionFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(expression->SourceLoc(), FakeSourceLoc(1)); ASSERT_EQ(expression->Tag(), Expression::Kind::TupleLiteral); EXPECT_THAT(cast(*expression).Fields(), IsEmpty()); @@ -48,8 +48,8 @@ TEST_F(ExpressionTest, EmptyAsExpression) { TEST_F(ExpressionTest, EmptyAsTuple) { ParenContents contents = {.elements = {}, .has_trailing_comma = false}; - Ptr tuple = TupleExpressionFromParenContents( - PtrTo(arena), FakeSourceLoc(1), contents); + Ptr tuple = + TupleExpressionFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(tuple->SourceLoc(), FakeSourceLoc(1)); ASSERT_EQ(tuple->Tag(), Expression::Kind::TupleLiteral); EXPECT_THAT(cast(*tuple).Fields(), IsEmpty()); @@ -68,7 +68,7 @@ TEST_F(ExpressionTest, UnaryNoCommaAsExpression) { .has_trailing_comma = false}; Ptr expression = - ExpressionFromParenContents(PtrTo(arena), FakeSourceLoc(1), contents); + ExpressionFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(expression->SourceLoc(), FakeSourceLoc(2)); ASSERT_EQ(expression->Tag(), Expression::Kind::IntLiteral); } @@ -79,8 +79,8 @@ TEST_F(ExpressionTest, UnaryNoCommaAsTuple) { .term = arena.New(FakeSourceLoc(2), 42)}}, .has_trailing_comma = false}; - Ptr tuple = TupleExpressionFromParenContents( - PtrTo(arena), FakeSourceLoc(1), contents); + Ptr tuple = + TupleExpressionFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(tuple->SourceLoc(), FakeSourceLoc(1)); ASSERT_EQ(tuple->Tag(), Expression::Kind::TupleLiteral); EXPECT_THAT(cast(*tuple).Fields(), @@ -94,7 +94,7 @@ TEST_F(ExpressionTest, UnaryWithCommaAsExpression) { .has_trailing_comma = true}; Ptr expression = - ExpressionFromParenContents(PtrTo(arena), FakeSourceLoc(1), contents); + ExpressionFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(expression->SourceLoc(), FakeSourceLoc(1)); ASSERT_EQ(expression->Tag(), Expression::Kind::TupleLiteral); EXPECT_THAT(cast(*expression).Fields(), @@ -107,8 +107,8 @@ TEST_F(ExpressionTest, UnaryWithCommaAsTuple) { .term = arena.New(FakeSourceLoc(2), 42)}}, .has_trailing_comma = true}; - Ptr tuple = TupleExpressionFromParenContents( - PtrTo(arena), FakeSourceLoc(1), contents); + Ptr tuple = + TupleExpressionFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(tuple->SourceLoc(), FakeSourceLoc(1)); ASSERT_EQ(tuple->Tag(), Expression::Kind::TupleLiteral); EXPECT_THAT(cast(*tuple).Fields(), @@ -124,7 +124,7 @@ TEST_F(ExpressionTest, BinaryAsExpression) { .has_trailing_comma = true}; Ptr expression = - ExpressionFromParenContents(PtrTo(arena), FakeSourceLoc(1), contents); + ExpressionFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(expression->SourceLoc(), FakeSourceLoc(1)); ASSERT_EQ(expression->Tag(), Expression::Kind::TupleLiteral); EXPECT_THAT(cast(*expression).Fields(), @@ -139,8 +139,8 @@ TEST_F(ExpressionTest, BinaryAsTuple) { .term = arena.New(FakeSourceLoc(3), 42)}}, .has_trailing_comma = true}; - Ptr tuple = TupleExpressionFromParenContents( - PtrTo(arena), FakeSourceLoc(1), contents); + Ptr tuple = + TupleExpressionFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(tuple->SourceLoc(), FakeSourceLoc(1)); ASSERT_EQ(tuple->Tag(), Expression::Kind::TupleLiteral); EXPECT_THAT(cast(*tuple).Fields(), diff --git a/executable_semantics/ast/pattern_test.cpp b/executable_semantics/ast/pattern_test.cpp index 5f8d5bbfd329..f69b4e066a5e 100644 --- a/executable_semantics/ast/pattern_test.cpp +++ b/executable_semantics/ast/pattern_test.cpp @@ -38,7 +38,7 @@ TEST_F(PatternTest, EmptyAsPattern) { ParenContents contents = {.elements = {}, .has_trailing_comma = false}; Ptr pattern = - PatternFromParenContents(PtrTo(arena), FakeSourceLoc(1), contents); + PatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(pattern->SourceLoc(), FakeSourceLoc(1)); ASSERT_TRUE(isa(*pattern)); EXPECT_THAT(cast(*pattern).Fields(), IsEmpty()); @@ -48,7 +48,7 @@ TEST_F(PatternTest, EmptyAsTuplePattern) { ParenContents contents = {.elements = {}, .has_trailing_comma = false}; Ptr tuple = - TuplePatternFromParenContents(PtrTo(arena), FakeSourceLoc(1), contents); + TuplePatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(tuple->SourceLoc(), FakeSourceLoc(1)); EXPECT_THAT(tuple->Fields(), IsEmpty()); } @@ -66,7 +66,7 @@ TEST_F(PatternTest, UnaryNoCommaAsPattern) { .has_trailing_comma = false}; Ptr pattern = - PatternFromParenContents(PtrTo(arena), FakeSourceLoc(1), contents); + PatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(pattern->SourceLoc(), FakeSourceLoc(2)); ASSERT_TRUE(isa(*pattern)); } @@ -78,7 +78,7 @@ TEST_F(PatternTest, UnaryNoCommaAsTuplePattern) { .has_trailing_comma = false}; Ptr tuple = - TuplePatternFromParenContents(PtrTo(arena), FakeSourceLoc(1), contents); + TuplePatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(tuple->SourceLoc(), FakeSourceLoc(1)); EXPECT_THAT(tuple->Fields(), ElementsAre(AutoFieldNamed("0"))); } @@ -90,7 +90,7 @@ TEST_F(PatternTest, UnaryWithCommaAsPattern) { .has_trailing_comma = true}; Ptr pattern = - PatternFromParenContents(PtrTo(arena), FakeSourceLoc(1), contents); + PatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(pattern->SourceLoc(), FakeSourceLoc(1)); ASSERT_TRUE(isa(*pattern)); EXPECT_THAT(cast(*pattern).Fields(), @@ -104,7 +104,7 @@ TEST_F(PatternTest, UnaryWithCommaAsTuplePattern) { .has_trailing_comma = true}; Ptr tuple = - TuplePatternFromParenContents(PtrTo(arena), FakeSourceLoc(1), contents); + TuplePatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(tuple->SourceLoc(), FakeSourceLoc(1)); EXPECT_THAT(tuple->Fields(), ElementsAre(AutoFieldNamed("0"))); } @@ -118,7 +118,7 @@ TEST_F(PatternTest, BinaryAsPattern) { .has_trailing_comma = true}; Ptr pattern = - PatternFromParenContents(PtrTo(arena), FakeSourceLoc(1), contents); + PatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(pattern->SourceLoc(), FakeSourceLoc(1)); ASSERT_TRUE(isa(*pattern)); EXPECT_THAT(cast(*pattern).Fields(), @@ -134,7 +134,7 @@ TEST_F(PatternTest, BinaryAsTuplePattern) { .has_trailing_comma = true}; Ptr tuple = - TuplePatternFromParenContents(PtrTo(arena), FakeSourceLoc(1), contents); + TuplePatternFromParenContents(&arena, FakeSourceLoc(1), contents); EXPECT_EQ(tuple->SourceLoc(), FakeSourceLoc(1)); EXPECT_THAT(tuple->Fields(), ElementsAre(AutoFieldNamed("0"), AutoFieldNamed("1"))); diff --git a/executable_semantics/common/ptr.h b/executable_semantics/common/ptr.h index 6960ca67ffa1..32842bf6e872 100644 --- a/executable_semantics/common/ptr.h +++ b/executable_semantics/common/ptr.h @@ -5,44 +5,11 @@ #ifndef EXECUTABLE_SEMANTICS_COMMON_PTR_H_ #define EXECUTABLE_SEMANTICS_COMMON_PTR_H_ -#include -#include - -#include "common/check.h" - namespace Carbon { // A non-nullable pointer. Written as `Ptr` instead of `T*`. template -class Ptr { - public: - explicit Ptr(T* ptr) : ptr(ptr) { CHECK(ptr != nullptr); } - - template >* = nullptr> - Ptr(Ptr other) : ptr(other.Get()) {} - - Ptr(std::nullptr_t) = delete; - - Ptr(const Ptr& other) = default; - Ptr& operator=(const Ptr& rhs) = default; - - auto operator*() const -> T& { return *ptr; } - auto operator->() const -> T* { return ptr; } - - T* Get() const { return ptr; } - - friend auto operator==(Ptr lhs, Ptr rhs) { return lhs.ptr == rhs.ptr; } - friend auto operator!=(Ptr lhs, Ptr rhs) { return lhs.ptr != rhs.ptr; } - - private: - T* ptr; -}; - -template -auto PtrTo(T& obj) -> Ptr { - return Ptr(&obj); -} +using Ptr = T* _Nonnull __attribute__((nonnull)); } // namespace Carbon diff --git a/executable_semantics/interpreter/interpreter.cpp b/executable_semantics/interpreter/interpreter.cpp index 0bc96f52af7c..e5e389d22397 100644 --- a/executable_semantics/interpreter/interpreter.cpp +++ b/executable_semantics/interpreter/interpreter.cpp @@ -465,7 +465,7 @@ auto Interpreter::StepExp() -> Transition { } else { // { { v :: [][i] :: C, E, F} :: S, H} // -> { { v_i :: C, E, F} : S, H} - auto* tuple = dyn_cast(act->Results()[0].Get()); + auto* tuple = dyn_cast(act->Results()[0]); if (tuple == nullptr) { FATAL_RUNTIME_ERROR_NO_LINE() << "expected a tuple in field access, not " << *act->Results()[0]; @@ -578,7 +578,7 @@ auto Interpreter::StepExp() -> Transition { // TODO: Think about a cleaner way to cast between Ptr types. // (multiple TODOs) .function = Ptr( - cast(act->Results()[0].Get())), + cast(act->Results()[0])), .args = act->Results()[1], .loc = exp->SourceLoc()}; default: diff --git a/executable_semantics/interpreter/type_checker.cpp b/executable_semantics/interpreter/type_checker.cpp index 529c6b8a3b18..a4dc3f63f05c 100644 --- a/executable_semantics/interpreter/type_checker.cpp +++ b/executable_semantics/interpreter/type_checker.cpp @@ -601,7 +601,7 @@ auto TypeChecker::TypeCheckPattern(Ptr p, TypeEnv types, // TODO: Think about a cleaner way to cast between Ptr types. // (multiple TODOs) auto arguments = Ptr( - cast(arg_results.pattern.Get())); + cast(arg_results.pattern)); return {.pattern = arena->New( alternative.SourceLoc(), ReifyType(choice_type, alternative.SourceLoc()), @@ -922,8 +922,7 @@ auto TypeChecker::TypeOfClassDef(const ClassDefinition* sd, TypeEnv /*types*/, FATAL_COMPILATION_ERROR(binding->SourceLoc()) << "Struct members must have names"; } - const auto* binding_type = - dyn_cast(binding->Type().Get()); + const auto* binding_type = dyn_cast(binding->Type()); if (binding_type == nullptr) { FATAL_COMPILATION_ERROR(binding->SourceLoc()) << "Struct members must have explicit types"; @@ -993,7 +992,7 @@ auto TypeChecker::MakeTypeChecked(const Ptr d, TCExpression type_checked_initializer = TypeCheckExp(var.Initializer(), types, values); const auto* binding_type = - dyn_cast(var.Binding()->Type().Get()); + dyn_cast(var.Binding()->Type()); if (binding_type == nullptr) { // TODO: consider adding support for `auto` FATAL_COMPILATION_ERROR(var.SourceLoc()) diff --git a/executable_semantics/main.cpp b/executable_semantics/main.cpp index 06eb2728418a..504cb6941c62 100644 --- a/executable_semantics/main.cpp +++ b/executable_semantics/main.cpp @@ -32,7 +32,7 @@ int main(int argc, char* argv[]) { Carbon::Arena arena; std::variant ast_or_error = - Carbon::Parse(PtrTo(arena), input_file_name); + Carbon::Parse(&arena, input_file_name); if (auto* error = std::get_if(&ast_or_error)) { // Diagnostic already reported to std::cerr; this is just a return code. @@ -40,5 +40,5 @@ int main(int argc, char* argv[]) { } // Typecheck and run the parsed program. - Carbon::ExecProgram(PtrTo(arena), std::get(ast_or_error)); + Carbon::ExecProgram(&arena, std::get(ast_or_error)); }