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

[pgffor] "remember=\x as \lastx" without "(initially ...)" reports \lastx undefined #855

Closed
muzimuzhi opened this issue May 18, 2020 · 10 comments

Comments

@muzimuzhi
Copy link
Member

Brief outline of the bug
The pgf manual (3.1.5b, sec. 89, p. 1004) says

By default the value of <variable> is zero for the first iteration, however, the optional (initially <value>) statement, allows the <macro> to be initially defined as <value>.

In my understanding, if (initially <value>) is missing, <macro> is initially defined as 0. But <macro> is reported to be undefined.

Minimal working example (MWE)

\documentclass{article}
\usepackage{pgffor}

\begin{document}
% based on the example in doc of option "remember", with "(initially A)" removed
\foreach \x [remember=\x as \lastx] in {B,...,H}
  {$\overrightarrow{\lastx\x}$, }
\end{document}

The above example reports

! Undefined control sequence.
<argument> \lastx 
@hmenke
Copy link
Member

hmenke commented May 18, 2020

Possible patch (needs testing)

--- a/tex/generic/pgf/utilities/pgffor.code.tex
+++ b/tex/generic/pgf/utilities/pgffor.code.tex
@@ -737,6 +737,7 @@
     \pgfutil@append@tomacro{\pgffor@remember@code}{\noexpand\def\noexpand#2{#2}}%
     \def\pgffor@test{#4}%
     \ifx\pgffor@test\pgfutil@empty%
+        \pgfutil@append@tomacro{\pgffor@assign@once@code}{\def#2{0}}%
     \else%
         \pgfutil@append@tomacro{\pgffor@assign@once@code}{\def#2{#4}}%
     \fi%

@hmenke hmenke added the pgffor label May 18, 2020
hmenke added a commit to hmenke/pgf that referenced this issue May 18, 2020
@hmenke
Copy link
Member

hmenke commented May 18, 2020

I think this was broken by this commit e22a1aa which means it has been broken for 10 years.

@hmenke
Copy link
Member

hmenke commented May 18, 2020

Interesting, I discovered an undocumented syntax in there:

\documentclass{article}
\usepackage{pgffor}
\begin{document}
\def\lastx{A}
\foreach \x/\y [remember={\lastx=\x}] in {B,...,H}
  {$\overrightarrow{\lastx\x}$, }
\end{document}

But that really doesn't work so well. The following behaves a little bit funky:

\documentclass{article}
\usepackage{pgffor}
\begin{document}
\def\lastx{A}
\def\lasty{0}
\foreach \x/\y [remember={\lastx=\x;\lasty=\y;}] in {B/1,...,H/6}
  {$\overrightarrow{\lastx\x\lasty\y}$, }
\end{document}

In fact, if I write \lastx=\x; \lasty=\y; (note the space before \lasty) it blows up with

! Missing control sequence inserted.
<inserted text> 
                \inaccessible 
l.10   {$\overrightarrow{\lastx\x\lasty\y}$, }
                                              
?

@hmenke hmenke added this to the 3.1.6 milestone May 18, 2020
@hmenke hmenke closed this as completed May 18, 2020
@muzimuzhi
Copy link
Member Author

muzimuzhi commented May 18, 2020

Possible patch (needs testing)

--- a/tex/generic/pgf/utilities/pgffor.code.tex
+++ b/tex/generic/pgf/utilities/pgffor.code.tex
@@ -737,6 +737,7 @@
     \pgfutil@append@tomacro{\pgffor@remember@code}{\noexpand\def\noexpand#2{#2}}%
     \def\pgffor@test{#4}%
     \ifx\pgffor@test\pgfutil@empty%
+        \pgfutil@append@tomacro{\pgffor@assign@once@code}{\def#2{0}}%
     \else%
         \pgfutil@append@tomacro{\pgffor@assign@once@code}{\def#2{#4}}%
     \fi%

The current fix 08041e4 (additionally) allows input with empty <value> in (initially <value>), see

\documentclass{article}
\usepackage{pgffor}

\begin{document}
% \lastx is initialized to zero as well
\foreach \x [remember=\x as \lastx (initially )] in {B,...,H}
  {$\overrightarrow{\lastx\x}$, }
\end{document}

That might not be the expected effect and will make the logic too complex to be fully documented. The following attempt avoids that:

--- a/tex/generic/pgf/utilities/pgffor.code.tex
+++ b/tex/generic/pgf/utilities/pgffor.code.tex
@@ -727,7 +727,7 @@
 }

 \def\pgffor@remember@parse@old#1#2\pgffor@stop{%
-    \pgffor@remember@@parse@old#1#2\pgffor@stop as#1(initially )\pgffor@@stop}
+    \pgffor@remember@@parse@old#1#2\pgffor@stop as#1(initially 0)\pgffor@@stop}

 \def\pgffor@remember@@parse@old#1#2as#3#4\pgffor@@stop{%
     \pgffor@remember@@@parse@old{#1}{#3}#2#4\pgffor@stop\pgffor@@stop}

hmenke pushed a commit to hmenke/pgf that referenced this issue May 18, 2020
@hmenke
Copy link
Member

hmenke commented May 18, 2020

Your proposed patch is much better. I have queued it for inclusion.

@muzimuzhi
Copy link
Member Author

Many thanks. :)

@muzimuzhi
Copy link
Member Author

A follow-up question: Do we need to allow \lastx to be manually initialized to empty?

  • In \foreach \x in <list>, empty item is not removed from <list>, hence \x is allowed to be empty for corresponding iteration(s).
  • In \foreach \x[remember=\x as \lastx (initially )] in <list>, \lastx is undefined for the first iteration, rather than initialized to empty.

If \lastx is allowed to be manually initialized to empty, then the following attempt is helpful:

--- a/tex/generic/pgf/utilities/pgffor.code.tex
+++ b/tex/generic/pgf/utilities/pgffor.code.tex
@@ -735,11 +735,7 @@
 \def\pgffor@remember@@@parse@old#1#2#3(initially #4)#5\pgffor@stop#6\pgffor@@stop{%
     \pgfutil@append@tomacro{\pgffor@assign@after@code}{\edef#2{#1}}%
     \pgfutil@append@tomacro{\pgffor@remember@code}{\noexpand\def\noexpand#2{#2}}%
-    \def\pgffor@test{#4}%
-    \ifx\pgffor@test\pgfutil@empty%
-    \else%
-        \pgfutil@append@tomacro{\pgffor@assign@once@code}{\def#2{#4}}%
-    \fi%
+    \pgfutil@append@tomacro{\pgffor@assign@once@code}{\def#2{#4}}%
 }

This changes the logic to (in pseudo-code)

if (initially <value>) is missing
    use 0 for <value>
else
    use <value>, no matter it is empty or not

Full example

\documentclass{article}
\usepackage{pgffor}

\begin{document}
% this prints "<>, <1>, <2>, <3>, <>,"
\foreach \x in {,1,2,3,} { <\x>, }

% currently, this raises an error "\lasti is undefined"
% if the attempt applied, \lastx is initialized to empty 
%   and this prints "<1>, <12>, <23>,", 
\foreach \x[remember=\x as \lastx (initially )] in {1,2,3}  { <\lastx\x>, }
\end{document}

hmenke pushed a commit to hmenke/pgf that referenced this issue May 21, 2020
@hmenke
Copy link
Member

hmenke commented May 21, 2020

If you are posting a patch, please always paste the full diff. The one that you posted is corrupt because there are three more lines missing, so I couldn't directly apply it and had to fix it by hand.

In fact, you don't have to patches at all, you can just file a pull request here on GitHub or you can use git send-email to post your patches to the mailing list at [email protected]

@muzimuzhi
Copy link
Member Author

Thanks again. I'll take care of it. Also, I'm not confident enough to directly open a pr.

@Mo-Gul
Copy link
Contributor

Mo-Gul commented May 22, 2020

@muzimuzhi, just give it a try. That will be a good chance to learn something.
(I regularly have messed up things and I am still here...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants