Implement Prism -> Sorbet conversion for True, False, and If nodes
#31
Conversation
Relocate it to the `PM_STATEMENTS_NODE` case so it can be reused by multiple parent nodes.
|
|
||
| auto size = stmts->body.size; | ||
| return convertPrismToSorbet(reinterpret_cast<pm_node *>(programNode->statements), parser, gs); |
There was a problem hiding this comment.
Wow GitHub chose the most confusing way to display this diff. I have not changed anything about the handling of PM_RATIONAL_NODE. I've moved most of the logic that was under PM_PROGRAM_NODE into PM_STATEMENTS_NODE.
| @@ -157,7 +157,7 @@ core::LocOffsets locOffset(pm_location_t *loc, pm_parser_t *parser) { | |||
| return core::LocOffsets{locStart, locEnd}; | |||
| } | |||
|
|
|||
| const unique_ptr<parser::Node> convertPrismToSorbet(pm_node_t *node, pm_parser_t *parser, core::GlobalState &gs) { | |||
| unique_ptr<parser::Node> convertPrismToSorbet(pm_node_t *node, pm_parser_t *parser, core::GlobalState &gs) { | |||
There was a problem hiding this comment.
I have a C++ question here -- the reason I removed this const is because I was having trouble reassigning ifTrue on line 185 below. Is there a better way to write the code below such that I wouldn't have to remove const here?
There was a problem hiding this comment.
Does it error because of the std::move(ifTrue) later?
There was a problem hiding this comment.
It errors because of the ifTrue = convertPrismToSorbet... It turns out that methods declared as const return things that can't be re-assigned. I think removing const is the right move after all.
| @@ -157,7 +157,7 @@ core::LocOffsets locOffset(pm_location_t *loc, pm_parser_t *parser) { | |||
| return core::LocOffsets{locStart, locEnd}; | |||
| } | |||
|
|
|||
| const unique_ptr<parser::Node> convertPrismToSorbet(pm_node_t *node, pm_parser_t *parser, core::GlobalState &gs) { | |||
| unique_ptr<parser::Node> convertPrismToSorbet(pm_node_t *node, pm_parser_t *parser, core::GlobalState &gs) { | |||
There was a problem hiding this comment.
Does it error because of the std::move(ifTrue) later?
amomchilov
changed the title
True, False, and If nodes
| @@ -0,0 +1,3 @@ | |||
| # typed: strict | |||
|
|
|||
| false | |||
There was a problem hiding this comment.
Honestly we should have just stopped there 😆
| if (ifNode->statements == nullptr) { | ||
| ifTrue = nullptr; | ||
| } else { | ||
| ifTrue = convertPrismToSorbet(reinterpret_cast<pm_node *>(ifNode->statements), parser, gs); |
There was a problem hiding this comment.
I think you need to move the result into ifTrue:
| ifTrue = convertPrismToSorbet(reinterpret_cast<pm_node *>(ifNode->statements), parser, gs); | |
| ifTrue = std::move(convertPrismToSorbet(reinterpret_cast<pm_node *>(ifNode->statements), parser, gs)); |
Not sure though, give it a shot
There was a problem hiding this comment.
This gets me: Moving a temporary object prevents copy elision (fix available).
When I add back the const in the method definition, then the error becomes: Object of type 'std::unique_ptr<parser::Node>' cannot be assigned because its copy assignment operator is implicitly deleted regardless of whether or not I call std::move.
😫 C++ why!
There was a problem hiding this comment.
ChatGPT recommends removing const from the method definition so I might just keep that:
Remove const from the Return Type: The typical use case for returning a unique_ptr is to transfer ownership of the dynamically allocated object to the caller. The unique_ptr itself being const is not usually necessary or helpful because you want to allow the caller to own and possibly modify (or reassign) the pointer. The immutability of the pointed-to object (if necessary) should be expressed through the pointed-to type (e.g., unique_ptr), not through the unique_ptr itself.
amomchilov
changed the title
True, False, and If nodesTrue, False, and If nodes
Best read commit-by-commit.
Motivation
This PR implements the conversion between Prism and Sorbet for True, False, and If node types.
If functionality is limited at the moment -- else/elsif clauses are not supported yet, but I wanted to get feedback on my approach before doing more.
Closes #87
Closes #102
Closes #174
Test plan
See included automated tests.