fix: check for \relax in \pgfkeysifdefined #1132
Conversation
hmenke
force-pushed
the
ifcsname
branch
3 times, most recently
from
e10fbbe to
136fe0d
Compare
hmenke
force-pushed
the
ifcsname
branch
2 times, most recently
from
49695b7 to
9ddda00
Compare
|
@hmenke my bad, please revert my last force push |
| \let\pgfkeys@relax\relax | ||
| \long\def\pgfkeysvalueof#1{\csname\pgfkeys@ifcsname{pgfk@#1}{pgfk@#1}{pgfkeys@relax}\endcsname} |
There was a problem hiding this comment.
I've added an alias for \relax for the unlikely event that someone actually does assign to this.
| given csname become `\relax` in case it wasn't defined. This resulted in some | ||
| leakage of accidentally `\relax`ed keys into the global scope. This has been | ||
| cleaned up a little. To preserve compatibility macros that used to expand to a | ||
| `\relax`ed csname now expand to a primitive `\relax`. This affects the |
There was a problem hiding this comment.
Outdated. Was changed to expand to \pgfkeys@relax (same four lines below, the last parts of this entry remain true however, this is a breaking change and one shouldn't make assignments to the results without testing for \relax or \pgfkeys@relax).
There was a problem hiding this comment.
Also: Should this get documented for \pgfkeysvalueof or should the documentation remain unchanged?
testfiles/gh-issue-1131.lvt
Outdated
| \ENDTEST | ||
|
|
||
| \BEGINTEST{pgfkeys: keys must be able to hold \relax} | ||
| \pgfkeyslet{/bar}{\relax} % or \pgfkeys{/bar/.initial=\relax} |
There was a problem hiding this comment.
Regarding the comment: \pgfkeys{/bar/.initial=\relax} doesn't \let to \relax but does \def\<key-path>{\relax}, so <key-path> doesn't have the \meaning \relax. More specifically: I'd expect \pgfkeysifdefined being false for \pgfkeyslet{<path>}{\relax} but true for \pgfkeys{<path>/.initial=\relax}. Maybe add a test for this.
There was a problem hiding this comment.
I don't think letting a key to \relax should “undefine” it. For that it's better to let it to something that is actually undefined.
There was a problem hiding this comment.
@hmenke agreed that \pgfkeyslet{/bar}{\myundefinedmacro} is better, but \let\foo\relax will have the same result for the majority of tests (and for the tests that have been implemented by now in pgfkeys).
There was a problem hiding this comment.
\pgfkeysifdefined has been using \ifcsname since 2007. I've not actually changed \pgfkeysifdefined but just cleaned up the accidental \relaxing. Now that I have squashed all the commits this should also be clearer.
There are a lot of instances in pgfkeys where \csname is used, e.g. in \pgfkeysgetvalueof. This will \let the mentioned csname become \relax so we have to be prepared for this in across the board in pgfkeys. Indeed, this was the original behavior before eTeX but it somehow got lost when transitioning to \ifcsname. The behavior is broken with eTeX since 641a883. BREAKING CHANGE: \pgfkeysvalueof will expand to an alias for the primitve \relax in case the key was undefined rather than to a \relaxed csname. Co-authored-by: Jonathan Spratte <jspratte@yahoo.de> Co-authored-by: muzimuzhi <muzimuzhi@gmail.com> Signed-off-by: Henri Menke <henri@henrimenke.de>
Motivation for this change
Fixes #1131
There are a lot of instances in pgfkeys where
\csnameis used, e.g. in\pgfkeysgetvalueof. This will\letthe mentioned csname become\relaxso we have to be prepared for this in\pgfkeysifdefined. Indeed, this was the original behavior before eTeX but it somehow got lost when transitioning to\ifcsname. The behavior is broken with eTeX since 641a883.TODO:
Checklist
Please signoff your commits to explicitly state your agreement to the Developer Certificate of Origin. If that is not possible you may check the boxes below instead: