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

Math refactor #871

Merged
merged 3 commits into from
Mar 25, 2024
Merged

Math refactor #871

merged 3 commits into from
Mar 25, 2024

Conversation

ollpu
Copy link
Collaborator

@ollpu ollpu commented Mar 25, 2024

There are three changes here. (1) could be done without the other two to fix the bug.

(2) & (3) might have some small performance implications in some cases. However, they should make it easier to disallow empty display math items $$$$, which I'm planning to do in a follow-up PR.

  1. Populate math_delims fully instead of stopping at an invalid closing delimiter. Inline code already works like this, though it can't encounter invalid delimiters.

    Fixes a bug when parsing e.g. this: $x {$ $ } $x$

  2. Disallow $$ matching a closing $ and then marching delimiters in make_math_span. Instead, retry scanning at the second position (really just a math_delims.find()).

    Before:

    $$x$
    ^--|    # make_math_span called with this span
    

    After:

    $$x$
    ^--|    # no match, move forward
     ^-|    # make_math_span called with this span
    
    
  3. Remove the seen_first optimization from MathDelims. It doesn't work with the retry strategy (2).

- Disallow $$ matching a closing $ and then marching delimiters in
  `make_math_span`. Instead, retry scanning at the second position.

- Remove the `seen_first` optimization from `MathDelims`. It doesn't
  work with the retry strategy.
@ollpu
Copy link
Collaborator Author

ollpu commented Mar 25, 2024

Ok, apparently I was mistaken about commonmark-hs disallowing empty display math items ($$$$). Let's not change that then.

How do we feel about changes 2 & 3? They simplify the overall logic and make_math_span quite a bit, but they're not really necessary at this point.

@ollpu ollpu requested a review from notriddle March 25, 2024 14:26
Copy link
Collaborator

@notriddle notriddle left a comment

Choose a reason for hiding this comment

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

Overall, I love how much simpler this is! Fix the silly nitpick in the test case, then r=me

pulldown-cmark/specs/math.txt Outdated Show resolved Hide resolved
Co-authored-by: Michael Howell <michael@notriddle.com>
@ollpu ollpu merged commit 74aed2f into pulldown-cmark:branch_0.11 Mar 25, 2024
1 check passed
@ollpu ollpu deleted the math-refactor branch March 25, 2024 15:42
@Martin1887
Copy link
Collaborator

Perfect, thanks!

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.

None yet

3 participants