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

Delimit if/else branches with braces instead of \else #36

Merged
merged 1 commit into from
Feb 24, 2020
Merged

Delimit if/else branches with braces instead of \else #36

merged 1 commit into from
Feb 24, 2020

Conversation

FrnchFrgg
Copy link
Contributor

@FrnchFrgg FrnchFrgg commented Jun 13, 2018

When cleveref 0.21.4 is loaded together with fmtcount 3.05 with the french language, e.g with

\documentclass[12pt,french]{article}
\usepackage{babel}
\usepackage{cleveref}
\usepackage{fmtcount}
\begin{document}
\ordinalstringnum{1}
\end{document}

the result is

Incomplete \ifnum; all text was ignored after line ...

This is probably due to a command like \@tempa or similar that cleveref redefines to \iftrue or \iffalse somewhere for its usage, which leads the fast conditional skipping of TeX astray. Use braced arguments and \@firstoftwo/\@secondoftwo instead, which fixes the problem.

@vincentb1 vincentb1 self-assigned this Jun 18, 2018
@vincentb1
Copy link
Owner

vincentb1 commented Jun 18, 2018

@FrnchFrgg Bonjour Julien,
Désolé, mais je n'arrive pas à reproduire le problème. Chez moi l'exemple minimal que vous donnez compile normalement…
Pourriez-vous clarifier quelles versions de fmtcount et de cleveref vous utilisez ?

Ci-joint l'essai que j'ai fait. Pour fmtcount à dire vrai je n'ai pas utilisé le HEAD de master, mais le HEAD de version_3.05+, je doute toutefois que cela fasse une différence, parce que fc-french.def et fmtcount.sty sont sensiblement identiques entre les deux.
french-frog-bug.zip

@vincentb1
Copy link
Owner

Ce commentaire contient les instructions pour récupérer et installer le HEAD de version_3.05+, histoire qu'on soit exactement sur la même version…

@FrnchFrgg
Copy link
Contributor Author

FrnchFrgg commented Jun 18, 2018

I can reproduce the bug with the minimal example zip you gave. I use LuaLaTeX, cleveref 0.21.4 2018/03/27, fmtcount v3.05 2017/12/24.
I also tested with the version_3.05+ branch and I get the same results. Similarly with straight pdfTeX instead of LuaTeX.

@vincentb1
Copy link
Owner

Sorry, I thought that you were a French speaking person. That is why I answered in French.

My cleveref version is as follows: « cleveref 2013/12/28 v0.19 Intelligent cross-referencing », that is probably why I could not reproduce the problem, I will update it and see what happens.

@FrnchFrgg
Copy link
Contributor Author

FrnchFrgg commented Jun 19, 2018

En effet, je suis français. Sauf que je parle tellement anglais sur le net que par réflexe je réponds en anglais... Et en effet, le problème s'est déclaré avec la mise à jour de TeXLive et donc de cleveref. Avec un ancien cleveref, pas de problème, c'est pour cela que j'ai précisé la version dans la pull request initiale.

@vincentb1
Copy link
Owner

OK, j'arrive à reproduire le bogue avec la dernière cleveref (la 0.21.5, dispo sur CTAN), et vous avez vu juste c'est bien le fait que cleveref redéfinisse à ìftrue ou \iffalse des noms de macro temporaire du genre \@tempf sans le faire au sein d'un groupe, ou sans ràzer \@tempf à la fin — par ex. avec un petit \let\@tempf\relax, qui pose le problème.

Je pense que le souci est plutôt dans cleveref. J'ai fait une modif que je vous joins…

@vincentb1
Copy link
Owner

cleveref.tar.gz

Et voilà…

@FrnchFrgg
Copy link
Contributor Author

Il y a un problème dans cleveref sans doute, mais il est aussi certain que les constructions avec des booléens primitifs \if \else \fi` de TeX sont très très fragiles dès lors qu'il peut y avoir du contenu non maitrisé à l'intérieur. Plusieurs solutions donc:

  1. Ne pas utiliser de variables « publiques » comme \@tempxxx dans le code mais uniquement des variables dont le nom fait qu'elles ont très très peu de probabilité d'être modifiées par un autre package. LaTeX3 (l3kernel, expl3) résoud en partie ce problème en fournissant des variables de type temporaire qui sont « typées » c'est-à-dire qui ont vocation à toujours recevoir le même type de contenu (et donc pas \iffalse ou \iftrue).
  2. Remplacer les structures conditionnelles primitives par des structures qui prennent des arguments. C'est globalement le choix de LaTeX2e avec les tests du genre \@ifdefined ou \@ifnextchar et cela a été formalisé par LaTeX3 avec les arguments de type TF. Ce choix est le plus logique.
    Bien sûr, cleveref devrait utiliser (comme dans LaTeX3) des variables définies à 0ou 1 au lieu d'utiliser \iffalse et \iftrue comme valeur booléenne, et remplacer l'appel direct au booléen par \if\@tempa1 par exemple. Mais rendre fmtcount robuste contre les idées farfelues de tous les autres concepteurs de packages semble justifié.

@vincentb1
Copy link
Owner

J'avoue que je ne suis pas complètement d'accord avec vos recommandations. Le problème c'est que cleveref fait un \let d'une variable temporaire \@tempf sur un conditionel \iftrue ou \iffalse. Forcément ça pose problème, et je suis sûr qu'il y a plein d'autres paquetages que fmtcount qui seront impactés. Donc je ne vais pas changer mon code pour contourner le problème causé par qq'un d'autre, en cachant ce problème, ce qui revient à retarder sa correction par son auteur.

Vous proposez d'utiliser des noms très peu probables, mais pour moi ça revient à encombrer inutilement la table de hashage de TeX, je préfère utiliser des noms communs pour les variables temporaires pour recycler les ressources mémoire déjà prises.

Par ailleurs, je n'étais pas satisfait de la 1re correction de cleveref que j'ai proposée, parce que ça revenait à admettre le \let de \@tempf sur un conditionnel comme qqchose de licite, en réparant le truc a posteriori par un \let\@tempf\@undefined. Je pense qu'il est plus propre d'utilser \if@tempswa. Voir le code macros2e sur le CTAN. Ci-joint donc la solution avec \if@tempswa:
cleveref.tar.gz

@vincentb1
Copy link
Owner

@FrnchFrgg Bonsoir,
Je m'aperçois que vous n'avez jamais répondu à mon dernier message. Êtes-vous satisfait de ma proposition de correction ? — si oui, vous pouvez vous l'approprier et la poster sur la forge de cleveref, personnellement je n'utilise pas ce paquetage.
Si non, alors vos commentaires sont les bien venus.

@FrnchFrgg
Copy link
Contributor Author

Je suis d'accord que le bug vient de cleveref. Cela dit, les concepteurs de LaTεX 2ε et surtout LaTεX 3 ont décidé consciemment d'éviter un maximum les constructions \if \else primitives, justement à cause de ces problèmes.

Une bonne manière d'être robuste contre les autres packages mal élevés est d'utiliser des techniques comme @FirstOfTwo en LaTεX2ε ou comme \bool_if:nTF en LaTεX3, qui s'affranchissent de ces problèmes graces aux arguments délimités par accolades.

Tel était le but de cette PR. Feel free to ignore it.

@vincentb1 vincentb1 changed the base branch from master to version_3.05+ February 24, 2020 13:43
When `cleveref 0.21.4` is loaded together with `fmtcount` 3.05 with the french language, e.g with
```
\documentclass[12pt,french]{article}
\usepackage{babel}
\usepackage{cleveref}
\usepackage{fmtcount}
\begin{document}
\ordinalstringnum{1}
\end{document}
```
the result is
```
Incomplete \ifnum; all text was ignored after line ...
```
This is probably due to a command like \@tempa or similar that cleveref redefines to \iftrue or \iffalse somewhere for its usage, which leads the fast conditional skipping of TeX astray. Use braced arguments and \@firstoftwo/\@secondoftwo instead, which fixes the problem.
@vincentb1 vincentb1 merged commit b3b5f6b into vincentb1:version_3.05+ Feb 24, 2020
@vincentb1
Copy link
Owner

@FrnchFrgg J'ai pris ta modif, mais comme master est en plein chantier, j'ai mis le changement vers la branche de maintenance version_3.05+ pour l'instant. Je refusionnerai tout ça dans master au moment de sortir la v4.00 qui comprend l'arabe.

@vincentb1
Copy link
Owner

@FrnchFrgg Merci pour la contrib.

@vincentb1
Copy link
Owner

@FrnchFrgg J'ai versé aujoiurd'hui sur le CTAN une version v3.07 qui prend en compte votre correction.

@FrnchFrgg
Copy link
Contributor Author

Thanks. I am still waiting for an answer from the cleveref maintainer (because making fmtcount more robust doesn't preclude from fixing the bug in the original package)

@FrnchFrgg FrnchFrgg deleted the fix-cleveref branch February 25, 2020 16:49
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