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

Add map helper #193

Draft
wants to merge 69 commits into
base: prism
Choose a base branch
from
Draft

Add map helper #193

wants to merge 69 commits into from

Conversation

amomchilov
Copy link

@amomchilov amomchilov commented Aug 15, 2024

Motivation

Ideally, I'd like to use std::views::transform to mimic Ruby's Array#map to simplify this "take some things, transform them, and collect the results" pattern, but it's not available until C++20/23.

We can use C++17's std::transform, but it's so over-parameterized that it's actually uglier than just writing the for loop yourself (and by an impressive margin!).

This PR is the next best thing, channelling some other-worldly-looking generics to create these mapInto and mapIntoNodeVec helpers.

Test plan

All existing tests pass.

egiurleo and others added 30 commits August 8, 2024 14:27
Instead of listing all the `srcs` and `hdrs` for the Prism C library,
use a glob to pull all `.c` and `.h` files, excluding the ones that are
generated by the genrule so that we can specify that the library
depends on that genrule without listing those files twice.
…et or prism

If `--parser=prism`, diverge in the `pipeline.cc` file and run a separate method that parses the source with prism.
This involves setting up a `convertPrismToSorbet` method that checks against every
possible node type. Program and Statements are basically skipped because Sorbet doesn't
use them, and then Integer is converted from a prism node to a Sorbet node.

Co-authored-by: Vinicius Stock <[email protected]>
1. Add a new test suite that only tests files in the `test/prism_regression` folder
2. Modify Sorbet's test code to allow the user to specify a parser
3. Add logic to the pipeline test runner to call `runPrismParser` when specified
Sorbet constructs slightly different ASTs depending on whether a program contains
one statement or more than one statements. Correctly parsing programs with more
than one statement will make it easier to benchmark this project.
In order to compare the performance of Prism with the Sorbet parser,
we need to be able to stop AST generation after Prism has run and
before we translate the Prism AST into the Sorbet AST.
These benchmarks will help us measure the progress of the prism in Sorbet project.
While they can be run from any machine, for "official" results, they should be run
on an AWS bare metal instance. Results should be added to the prism_benchmarks/data
directory.
Relocate it to the `PM_STATEMENTS_NODE` case so it can be reused
by multiple parent nodes.
@amomchilov amomchilov marked this pull request as ready for review August 15, 2024 21:33
@amomchilov amomchilov self-assigned this Aug 15, 2024
Copy link

@egiurleo egiurleo left a comment

Choose a reason for hiding this comment

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

Let's sell our souls to the C++ metaprogramming gods! 🔥

@@ -7,6 +7,8 @@ using std::unique_ptr;

namespace sorbet::parser::Prism {

namespace {

Choose a reason for hiding this comment

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

C++ newb question -- what does this empty namespace do here?

Copy link
Author

Choose a reason for hiding this comment

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

Great question. I'll let you know when I find out.

I just saw that unexported functions were put into anonymous namespaces like this in other files, so I cargo-culted it. :')

Copy link

Choose a reason for hiding this comment

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

These are "anonymous" namespaces. To a first approximation, anonymous namespaces are the equivalent of static: the enclosed names aren't exposed from the compilation unit (i.e. the .cc file).

Choose a reason for hiding this comment

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

Thanks @froydnj!

@amomchilov
Copy link
Author

Actually gonna take this back to draft, I have a few ideas to make it simpler, but it's lower priority so I'll come back to it later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants