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.

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

Cleanups in parser and parser tests. #473

Merged
merged 3 commits into from
Apr 21, 2021
Merged

Cleanups in parser and parser tests. #473

merged 3 commits into from
Apr 21, 2021

Conversation

zygoloid
Copy link
Contributor

Depends on #470.

@zygoloid zygoloid requested a review from fowles April 19, 2021 21:32
@zygoloid zygoloid requested a review from a team as a code owner April 19, 2021 21:32
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Apr 19, 2021
@github-actions github-actions bot added the dependent Depends on another issue/PR label Apr 19, 2021
@github-actions github-actions bot removed the dependent Depends on another issue/PR label Apr 20, 2021
@github-actions
Copy link

🎉 Great news! Looks like all the dependencies have been resolved:

💡 To add or remove a dependency please update this issue/PR description.

Brought to you by Dependent Issues (:robot: ). Happy coding!

@@ -316,6 +316,26 @@ auto MatchNode(Args... args) -> ExpectedNode {
return MatchNode(ParseNodeKind::kind(), args...); \
}
#include "parse_node_kind.def"

// Helper for matching a designator `lhs.rhs`.
auto MatchDesignator(ExpectedNode lhs, std::string rhs) -> ExpectedNode {
Copy link

Choose a reason for hiding this comment

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

const& or llvm::StringRef?

Copy link

Choose a reason for hiding this comment

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

or maybe std::move in the body

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added std::move here and also in the other matchers. (I'd not been worrying too much about copies in this test-only code but I suppose we could end up doing a lot of vector deep copies here.)

These contain `vector<ExpectedNode>`s, resulting in deep copies of
potentially large trees.
@zygoloid zygoloid merged commit b70fda9 into carbon-language:trunk Apr 21, 2021
@zygoloid zygoloid deleted the parser-cleanups branch March 11, 2022 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants