Use span to simplify looping #34
Conversation
| pm_node_t *node = stmts->body.nodes[i]; | ||
| for (auto& node : stmts) { | ||
| unique_ptr<parser::Node> convertedStmt = convertPrismToSorbet(node, parser, gs); | ||
| sorbetStmts.emplace_back(std::move(convertedStmt)); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
BTW would that lambda need a capture list for pm_parser_t *parser and core::GlobalState &gs?
There was a problem hiding this comment.
yeah, probably avoided because it is so ugly compared to for loop, as you said.
|
|
||
| for (int i = 0; i < stmts->body.size; i++) { | ||
| pm_node_t *node = stmts->body.nodes[i]; | ||
| for (auto& node : stmts) { |
Motivation
C lacks a standard abstraction for representing a buffer/array, so you're always just slinging around a separate pointer and size.
In C++, we have
std::vector, which brings us nice things like being able to range-forover it (for (auto& thing : things) { ... }). However,std::vectorowns its buffer, and there's no way to create one from a C buffer without a copy.To bridge the two worlds, C++ 20 offers
std::spanwhich gives you back an object with an iterable view over memory that it doesn't own. This let you easily wrap a pre-existing pointer+size, and get an iterable view over it. We're on C++ 17, so we can't use it, but there'sabsl::span, which can fill the gap.Test plan
This is a pure refactor, and this codepath is already tested.