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

Fix split_at (return Result ver.) #455

Closed
wants to merge 4 commits into from
Closed

Fix split_at (return Result ver.) #455

wants to merge 4 commits into from

Conversation

kyoheiu
Copy link
Contributor

@kyoheiu kyoheiu commented Nov 3, 2022

Fixed #453

See also another PR #456 .

This PR fixes split_at, which can panic when splitting occurs inside the middle of a wide character, to return Result to avoid panic.
Also includes some of rust-analyzer's formatting.

Test results:
commit 8678b7c

failures:
    highlighting::highlighter::tests::can_parse
    highlighting::highlighter::tests::can_parse_with_highlight_state_from_cache
    highlighting::highlighter::tests::test_ranges
    highlighting::theme_set::tests::can_parse_common_themes
    html::tests::strings
    html::tests::tricky_test_syntax
    parsing::parser::tests::can_compare_parse_states
    parsing::parser::tests::can_parse_backrefs
    parsing::parser::tests::can_parse_includes
    parsing::parser::tests::can_parse_issue25
    parsing::parser::tests::can_parse_preprocessor_rules
    parsing::parser::tests::can_parse_simple
    parsing::parser::tests::can_parse_yaml
    parsing::syntax_set::tests::can_load

test result: FAILED. 86 passed; 14 failed; 0 ignored; 0 measured; 0 filtered out; finished in 3.11s

fix-split_at-result

failures:
    highlighting::highlighter::tests::can_parse
    highlighting::highlighter::tests::can_parse_with_highlight_state_from_cache
    highlighting::highlighter::tests::test_ranges
    highlighting::theme_set::tests::can_parse_common_themes
    html::tests::strings
    html::tests::tricky_test_syntax
    parsing::parser::tests::can_compare_parse_states
    parsing::parser::tests::can_parse_backrefs
    parsing::parser::tests::can_parse_includes
    parsing::parser::tests::can_parse_issue25
    parsing::parser::tests::can_parse_preprocessor_rules
    parsing::parser::tests::can_parse_simple
    parsing::parser::tests::can_parse_yaml
    parsing::syntax_set::tests::can_load

test result: FAILED. 86 passed; 14 failed; 0 ignored; 0 measured; 0 filtered out; finished in 3.00s

which means nothing is changed by this PR except split_at and its test.

@kyoheiu kyoheiu changed the title Fix split_at (return Result ver.) Fix split_at (return Result ver.) Nov 3, 2022
@Enselic
Copy link
Collaborator

Enselic commented Nov 24, 2022

To you make your PRs easier to review, can you undo the formatting changes please?

(IMHO we should enforce cargo fmt in CI, which is a widely used practice across the Rust ecosystem, and works great in my experience. I would volunteer to set it up if there is interest.)

@kyoheiu
Copy link
Contributor Author

kyoheiu commented Nov 24, 2022

Sorry for the inconvenience: Reverted the formatting in 2 PRs I created.

@Enselic
Copy link
Collaborator

Enselic commented Jan 5, 2023

Sorry for keeping you waiting.

Before we can make progress here, we need to make CI green. I created a PR for that: #462

The test failures you see are because you did not clone submodules. Note in .github/workflows/CI.yml how submodules: true for actions/checkout@v2 in build-and-test.

Try running git submodule update --init before you run cargo test.

@kyoheiu kyoheiu closed this Jan 14, 2023
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.

util::split_at panics with multibyte characters
2 participants