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

[WIP] Feat: Add C++ #368

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

[WIP] Feat: Add C++ #368

wants to merge 13 commits into from

Conversation

jmqd
Copy link

@jmqd jmqd commented Jun 4, 2024

I had a few hours to take a swing at this. This is a time-boxed PR and still WIP.

Open questions:

  1. I still don't grok the patching the grammar business, e.g. when to use field vs bare choice, and also I'm not yet confident that when I change cpp-metavariable-grammar.js the changes are immediately reflected in cargo test, so I need to verify that my edit-test loop is correct (e.g. no cached artifacts or anything.)

  2. I wanted to add the following test, but it was failing. I assume it's because of (1) above. I assume grokking this one case will enable me to add and fix many similar grammar-related cases.

#[test]
fn cpp_use_braced_initialization() {
    run_test_expected({
        TestArgExpected {
            pattern: r#"
                |language cpp
                |`int $any = $rval;` => `int $any {$rval};`
                |"#
            .trim_margin()
            .unwrap(),
            source: r#"int num = 42;"#.to_owned(),
            expected: r#"int num {42};"#.to_owned(),
        }
    })
    .unwrap();
}

The failure symptom:

---- test::cpp_use_braced_initialization stdout ----
thread 'test::cpp_use_braced_initialization' panicked at crates/core/src/test.rs:15022:6:
called `Result::unwrap()` on an `Err` value: pattern failed to MATCH
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@morgante
Copy link
Contributor

morgante commented Jun 4, 2024

I still don't grok the patching the grammar business, e.g. when to use field vs bare choice, and also I'm not yet confident that when I change cpp-metavariable-grammar.js the changes are immediately reflected in cargo test, so I need to verify that my edit-test loop is correct (e.g. no cached artifacts or anything.)

Just to confirm, are you running the edit_grammars script from this document after making changes? You need to do that before running cargo test.

Your failing test is likely reflective of grammars not being updated.

@jmqd
Copy link
Author

jmqd commented Jun 4, 2024

I still don't grok the patching the grammar business, e.g. when to use field vs bare choice, and also I'm not yet confident that when I change cpp-metavariable-grammar.js the changes are immediately reflected in cargo test, so I need to verify that my edit-test loop is correct (e.g. no cached artifacts or anything.)

Just to confirm, are you running the edit_grammars script from this document after making changes? You need to do that before running cargo test.

Your failing test is likely reflective of grammars not being updated.

Thanks, this must be the issue. I wasn’t running that script.

@morgante
Copy link
Contributor

morgante commented Jun 4, 2024

Thanks, this must be the issue. I wasn’t running that script.

Please do read the documentation / contributing guide thoroughly. It covers important details.

@morgante
Copy link
Contributor

@jmqd Just checking - need any further advice on this?

@jmqd
Copy link
Author

jmqd commented Jun 11, 2024

@jmqd Just checking - need any further advice on this?

Hey @morgante,

I was able to spend another hour and change on this. I pushed those working changes. I got the edit_grammar stuff online, (required a little munging to play nice with nixos since the npx script tried to mutate the immutable /nix/store).

I have a few tests passing, but after a variety of iterations/combinations in the grammar file, I can't get gritql to match the identifier:

$ cargo test --workspace cpp_
running 3 tests
test test::cpp_change_float_to_double ... FAILED
test test::cpp_rename_variable_name ... ok
test test::cpp_simple ... ok

failures:

---- test::cpp_change_float_to_double stdout ----
thread 'test::cpp_change_float_to_double' panicked at crates/core/src/test.rs:15039:6:
called `Result::unwrap()` on an `Err` value: pattern failed to MATCH
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I'm relatively low confidence on how I did the conflict resolution, as it's my first time touching one of these JS grammar files. I tried various combinations of field, plain indirection via $._identifier and choice(), etc., but couldn't get a match.

LMK if you see anything obviously wrong in the grammar file specifically, unless there's some plumbing that I've missed. One of the weird parts about C++ is that the grammar is defined in terms of the C grammar, so I've brought both in and I'm making changes in the C grammar that should be reflected in the C++ grammar (due to dependency relationship), but it's possible that breaks a plumbing assumption somewhere else in the system.

Please do read the documentation / contributing guide thoroughly. It covers important details.

Thanks, appreciate the note here. I've also made note of a few places where it is insufficient so I can add to those docs. For example, making mention of the edit_grammar script into the Adding a Language section.

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.

2 participants