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

Minor refactor for \__tcobox_include_graphics:nn #236

Closed
muzimuzhi opened this issue Jun 19, 2023 · 9 comments
Closed

Minor refactor for \__tcobox_include_graphics:nn #236

muzimuzhi opened this issue Jun 19, 2023 · 9 comments
Assignees

Comments

@muzimuzhi
Copy link
Contributor

Current

\cs_new:Npn \__tcobox_include_graphics:nn #1#2
{
\tl_set:Nn \l_tmpa_tl { \includegraphics }
\tl_put_right:Nx \l_tmpa_tl { [#1] }
\l_tmpa_tl {#2}
}

Proposal

\cs_new_protected:Npn \__tcobox_include_graphics:nn #1#2
  {
    \includegraphics [#1] {#2}
  }
\cs_generate_variant:Nn \__tcobox_include_graphics:nn {e}

Then use \__tcobox_include_graphics:en in various places.

@T-F-S
Copy link
Owner

T-F-S commented Jun 19, 2023

Is there a specific reason why you would replace x expansion by e expansion?
The interface3.pdf documentation say that e can be more than 200 times slower than x. My impression is that this should only be used for expandable functions (?).

The proposed variant way is more elegant, but do you see any problem with the current implementation (besides the lack of elegance)?

@muzimuzhi
Copy link
Contributor Author

muzimuzhi commented Jun 19, 2023

The interface3.pdf documentation say that e can be more than 200 times slower than x.

This only happens if primitive \expanded is not available (the scary 200-times-slower conclusion came from comparing an emulation for \expanded with a \edef, not between the two primitives), which is generally available starting from TeX Live 2019. Considering tcolorbox has already required LaTeX2e 2020-10-01...

BTW, that \expanded emulation is removed from l3kernel recently, starting from l3kernel 2023-05-15.

The proposed variant way is more elegant, but do you see any problem with the current implementation (besides the lack of elegance)?

Nope :D (That's why it's called refactor, not fix.)

@T-F-S
Copy link
Owner

T-F-S commented Jun 19, 2023

This only happens if primitive \expanded is not available (the scary 200-times-slower conclusion came from comparing an emulation for \expanded with a \edef, not between the two primitives), which is generally available starting from TeX Live 2019. Considering tcolorbox has already required LaTeX2e 2020-10-01...

That is good to know, but, still, why use e instead of x here? \__tcobox_include_graphics is not expandable, so \__tcobox_include_graphics:xn would be more natural to me (or have I missed something?).

@muzimuzhi
Copy link
Contributor Author

e-type does the fully expansion "in-place", while x-type needs some intermediate form like \edef \temp {...} \temp. Personally when there's no need to half the #s in argument, I prefer e-type expansion. It's also slightly quicker than x-type.

On my laptop, running

\documentclass{article}
\usepackage{l3benchmark}
\ExplSyntaxOn
\benchmark:n { \exp_args:Ne \use_none:n {foo} }
\benchmark:n { \exp_args:Nx \use_none:n {foo} }
\ExplSyntaxOff

\begin{document}
\end{document}

gives (in log)

2.92e-7 seconds (1.24 ops)
4.55e-7 seconds (1.89 ops)

For a base function taking two n-type args like \myfunc:nn,

  • \cs_generate_variant:Nn \myfunc:nn {e} will do \cs_new:Npn \myfunc:en { \exp_args:Ne \myfunc:nn } or \cs_new_protected:Npn \myfunc:en { \exp_args:Ne \myfunc:nn }, depending on whether \myfunc:nn is expandable
  • \cs_generate_variant:Nn \myfunc:nn {x} will do \cs_new_protected:Npn \myfunc:xn { \exp_args:Nx \myfunc:nn } (because x-type expansion is never expandable)

Thus benchmarking \myfunc:en and \myfunc:xn is equivalent to benchmarking \exp_args:Ne and \exp_args:Ne.


LaTeX2e also generates both e- and x-type variants for non-expandable base functions, so I think it's more of a personal taste (?). For example, from texdoc source2e,

  • e-type variants
    • \cs_generate_variant:Nn \__hook_code_gset_auxi:nnnn { eeen }
    • \cs_generate_variant:Nn \__hook_code_gset:nn { ne }
  • x-type variants
    • \cs_generate_variant:Nn \__cmd_add_arg:n { V , o , x }

@T-F-S
Copy link
Owner

T-F-S commented Jun 20, 2023

Thank you for the explanations. I memorized something like "Only use 'e' when absolutely necessary to avoid computational overhead", but, obviously, nowadays it is the other way around. On my computer, I can confirm your results (pdflatex with current TeXLive).

So, the e description on page 3 of interface3.pdf is quite misleading for current engines.

I will keep that in mind for the future.

@T-F-S T-F-S self-assigned this Jun 20, 2023
@muzimuzhi
Copy link
Contributor Author

So, the e description on page 3 of interface3.pdf is quite misleading for current engines.

I've proposed latex3/latex3#1235.

@muzimuzhi
Copy link
Contributor Author

Reviewing corresponding changes in 4918308 for this issue, I started to worry about the fully-expansion made to graphics options, started from 0b857db (4.13, 2018-03-22), with changes entry

- library 'skins':
* Options given by '/tcb/graphics options' and '/tikz/fill image options'
are now fully expanded while applied to underlying '\includegraphics'

For \includegraphics, it's original options and the extensions from graphbox package should all be ok with an exhaustive expansion, but some from adjustbox package are not, thanks to the powerful color expression supported by xcolor. See an example below.

\protected@edef is safer, see similar changes made to latex3/latex2e#1140 in latex3/latex2e@4c73390.

The failing use case is so corner that I think tcolorbox need not adapt for it, instead can just document that /tcb/graphics options value will be fully expanded. The /tcb/fill image options now lives in https://github.com/T-F-S/tikzfill.

An example showing graphics options={cframe=<color> <rule width>} may fail

\documentclass{article}
\usepackage[OT1]{fontenc}
\usepackage[french]{babel}

% make \includegraphics able to use all \adjincludegraphics options
\usepackage[Export]{adjustbox}
\usepackage[skins]{tcolorbox}

\tcbset{nobeforeafter}

\begin{document}
% works
\includegraphics[width=2cm, cframe=blue!50 1pt]{example-image}

% works
\shorthandoff{!}
\tcbincludegraphics[hbox, graphics options={width=2cm, cframe=blue!50 1pt}]{example-image}
\shorthandon{!}

% errors due to active exclamation mark
\tcbincludegraphics[hbox, graphics options={width=2cm, cframe=blue!50 1pt}]{example-image}

% control group
\tcbincludegraphics[hbox, graphics options={width=2cm, cframe=blue 1pt}, title={control group}]{example-image}
\end{document}

image

@T-F-S
Copy link
Owner

T-F-S commented Sep 26, 2023

I will add Note that \meta{options} will be fully expanded to the documentation.

@T-F-S
Copy link
Owner

T-F-S commented Sep 28, 2023

Done with https://github.com/T-F-S/tcolorbox/releases/tag/v6.1.0 (last documentation change will come in a later version)

@T-F-S T-F-S closed this as completed Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants