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

Implement expanded-fix for keyval #912

Closed
wants to merge 1 commit into from

Conversation

Skillmon
Copy link
Contributor

This PR implements the \unexpanded\expanded fix for \keyval_parse:nnn to make it alignment safe. As a fall back, fix number 2 from #896 is used (with proper adjustments to the error recovery for blank key names).

I ran the testsuite locally on the code used if \expanded isn't available. One should be aware that the testsuite will henceforth only test the expanded variant (at least without manual modifications).

@blefloch
Copy link
Member

I ended up using parts of the ideas in this pull request but with modifications to keep the code more compact and avoid having twice the same code simply with extra \group_align_safe_begin:/_end:. Please take a look at commits ec1e3f9 and 05cc492 and check if they seem correct.

@blefloch blefloch closed this May 12, 2021
@Skillmon
Copy link
Contributor Author

I think it looks good, but only took a quick glance on my mobile. This evening I can take another look and give proper feedback.

@Skillmon
Copy link
Contributor Author

@blefloch took another look, all is looking good. Also, I think the definition of \@@_blank_key_error:w could be even more simplified, since the error-case doesn't have to be fast, imho. So the argument grabbing optimization there is really unnecessary. We could go with a single definition, namely:

\cs_new:Npn \@@_blank_key_error:w #1 \@@_loop_other:nnw
  {
    \__kernel_msg_expandable_error:nn { keyval } { blank-key-name }
    \@@_loop_other:nnw
  }

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

2 participants