Skip to content

Math refactor - #871

Merged
ollpu merged 3 commits into
pulldown-cmark:branch_0.11from
ollpu:math-refactor
Mar 25, 2024
Merged

ollpu merged 3 commits into
pulldown-cmark:branch_0.11from
ollpu:math-refactor

Conversation

@ollpu

@ollpu ollpu commented Mar 25, 2024

Copy link
Copy Markdown
Collaborator

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).

ollpu added 2 commits March 25, 2024 15:50
- 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

ollpu commented Mar 25, 2024 •

Copy link
Copy Markdown
Collaborator Author

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

@notriddle notriddle left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread pulldown-cmark/specs/math.txt Outdated
Co-authored-by: Michael Howell <[email protected]>
@ollpu
ollpu merged commit 74aed2f into pulldown-cmark:branch_0.11 Mar 25, 2024
@ollpu
ollpu deleted the math-refactor branch March 25, 2024 15:42
@Martin1887

Copy link
Copy Markdown
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.

3 participants