Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use span to simplify looping #34

Merged
merged 1 commit into from
Aug 1, 2024
Merged

Use span to simplify looping #34

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions main/pipeline/pipeline.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,23 +220,25 @@ unique_ptr<parser::Node> convertPrismToSorbet(pm_node_t *node, pm_parser_t *pars
return make_unique<parser::Rational>(locOffset(loc, parser), value);
}
case PM_STATEMENTS_NODE: {
pm_statements_node *stmts = reinterpret_cast<pm_statements_node *>(node);
pm_statements_node *stmts_node = reinterpret_cast<pm_statements_node *>(node);

auto stmts = absl::MakeSpan(stmts_node->body.nodes, stmts_node->body.size);

// For a single statement, do not create a Begin node and just return the statement
if (stmts->body.size == 1) {
return convertPrismToSorbet((pm_node *)stmts->body.nodes[0], parser, gs);
if (stmts.size() == 1) {
return convertPrismToSorbet((pm_node *)stmts.front(), parser, gs);
}

// For multiple statements, convert each statement and add them to the body of a Begin node
parser::NodeVec sorbetStmts;
sorbetStmts.reserve(stmts.size());

for (int i = 0; i < stmts->body.size; i++) {
pm_node_t *node = stmts->body.nodes[i];
for (auto& node : stmts) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

unique_ptr<parser::Node> convertedStmt = convertPrismToSorbet(node, parser, gs);
sorbetStmts.emplace_back(std::move(convertedStmt));
}
Comment on lines -234 to 239
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you could also do:

std::transform(stmts.begin(), stmts.end(), back_inserter(sorbetStmts), [](auto& node) { 
  return convertPrismToSorbet(node, parser, gs);
});

right? The return value optimization should move the return value into the sorbet statements node.

Copy link
Author

@amomchilov amomchilov Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm usually a huge fan of stream operations like map, but how did C++ manage to make map so verbose/heavy that even I almost think the manual for loop is nicer. It doesn't even call .reserve() for you. 😭

Anyways, std::transform seemed to be pretty rare in the codebase (only 2 usages). Is there some style preference against it? There's also absl::c_transform, which has 3 usages. Overall it looks like it was avoided, for some reason

Copy link
Author

@amomchilov amomchilov Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW would that lambda need a capture list for pm_parser_t *parser and core::GlobalState &gs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, probably avoided because it is so ugly compared to for loop, as you said.


auto *loc = &stmts->base.location;
auto *loc = &stmts_node->base.location;

return make_unique<parser::Begin>(locOffset(loc, parser), std::move(sorbetStmts));
}
Expand Down