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

handle empty case in Tokens::stripBraces #2457

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dginev
Copy link
Collaborator

@dginev dginev commented Dec 24, 2024

Fixes #2452

The classic loops in Tokens::stripBraces were missing an edge case where the braces held nothing - and an empty Tokens would be the appropriate response.

Interestingly, the bug reported in #2452 was absent from the Rust port, as I had used Rust's Iterator abstraction (with peekable), which safely avoids the need of index arithmetic.

@dginev dginev requested a review from brucemiller December 24, 2024 11:25
@dginev
Copy link
Collaborator Author

dginev commented Jan 9, 2025

I also added the report from the issue as a new test here, to spot any regressions (e.g. during porting)

\usepackage{keyval}
\makeatletter
\define@key{createvalue}{empty}{\def\myempty{#1}}
\setkeys{createvalue}{empty={}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest adding {{}} (and maybe {{{}}} too), to be sure that the correct number of braces is stripped out. When I reported the issue, I fixed it locally by stripping all braces, but that was wrong too.

Copy link
Collaborator Author

@dginev dginev Jan 9, 2025

Choose a reason for hiding this comment

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

Great point, the check only comes back empty for {} and {{}} in keyval.

The stripBraces fix is correct for the intended semantics of "stripBraces", but it looks like keyval should only strip them twice at most. So it should be called twice? ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, a little scary but I updated the test to request exactly 2 braced strips for this case, extending stripBraces to allow for a number of layers.

@dginev
Copy link
Collaborator Author

dginev commented Jan 9, 2025

Btw, there is a semi-related curiosity that I recently learned thanks to Norbert Preining, about there being a maximum number of strips in usepackage braces. If you look at the following example:

\documentclass{article}
\usepackage{{{{{{graphicx}}}}}} % works
\usepackage{{{{{{{graphicx}}}}}}} % fails

\begin{document}
oops.
\end{document}

pdflatex will successfully unwrap 6 braces, but will keep the seventh and try to load {graphicx}.sty in the second case. I consider it a feature that latexml does not emulate that accurately, and would succeed with both 6 and 7 braces. I also hope this is rarely used by real authors...

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.

empty group in keyval should return empty value
2 participants