Re: [vim/vim] transstr(): keep a tail pointer instead of appending with STRCAT() (PR #20925)
h_east (Vim Github Repository) <[email protected]> Mon, 03 Aug 2026 08:14:28 -0700
| Newsgroups | gmane.editors.vim.devel |
|---|---|
| Message-ID | <vim/vim/pull/20925/[email protected]> |
----==_mimepart_6a70b0544bc0f_f211802508eb
Content-Type: text/plain; charset="UTF-8"
h-east left a comment (vim/vim#20925)
The change itself is correct. `STRCAT()` rescans the result from the start
for every appended character, so the loop was O(n^2) in the length of the
result, and keeping a tail pointer makes it linear. Moving the terminating
NUL to after the loop is safe here because the loop has no early exit. The
tests need work though.
### The new test is not portable, Windows CI is red
Three assertions fail on the Windows runners. `strtrans("\x01\x1f\x7f")`
returns the 0x7f unchanged, and both cases expecting `<9f>` come back with a
literal 0x9f.
The cause is 'isprint'. `vim_isprintc()` decides printability of anything
below 0x100 from `g_chartab`, which is built from 'isprint', and the default
differs per platform: `"@,~-255"` on Win32 and VMS, `"@,161-255"` elsewhere.
So 0x7f and 0x9f are printable on Windows and `transstr()` correctly leaves
them alone there.
Two ways out. Either set 'isprint' in the test so the expectations hold
everywhere:
```vim
let save_isprint = &isprint
set isprint=@,161-255
...
let &isprint = save_isprint
```
Or drop the sub-0x100 cases and rely on the `nr2char(0x200b)` and
`nr2char(0xfeff)` assertions, which go through `utf_printable()` and are not
affected by 'isprint'. Those two already pass on every runner.
The first option is better if you want to keep covering the two-digit
`transchar_hex()` output, since `<200b>` and `<feff>` only exercise the
four-digit branch.
### Test coverage does not include the non-multi-byte path
The second commit says it covers all `strtrans()` code paths, but every test
runs with the default 'encoding'. `transstr()` branches on `has_mbyte` for
the size computation, and the `else` branch using `vim_strsize()` is never
reached.
`test_functions.vim` already switches encoding in several tests, so a block
like this would close the gap, with 'isprint' pinned for the same reason as
above:
```vim
let save_isprint = &isprint
set encoding=latin1
set isprint=@,161-255
call assert_equal('a^Mb', strtrans("a\rb"))
...
set encoding=utf-8
let &isprint = save_isprint
```
Please verify the expected strings on your side, the point is the branch
rather than the exact output.
### Prefer the pattern already used in this file
For the single-byte case the patch does `STRCPY()` followed by `STRLEN(d)`,
which walks the translated character twice. `trans_characters()` a few lines
above solves the same problem by taking the length first:
```c
trs = transchar_byte(*p++);
trs_len = (int)STRLEN(trs);
mch_memmove(d, trs, (size_t)trs_len);
d += trs_len;
```
That is one scan instead of two and matches the surrounding code. The strings
are short so this is not about speed, it is about staying consistent with the
file.
### Please add a measurement
The commit message says this could impact `get_foldtext()` in `fold.c`. The
complexity argument is sound on its own, but a before/after timing on a long
line would make the case concrete and would tell us whether this is a real
user-visible win or a theoretical one. Something like `strtrans()` on a
100000 byte line with a mix of control characters is enough.
### Out of scope, but worth noting
`transchar_hex()` already knows how many bytes it wrote in its local `i`, and
it returns void. If it returned that length, both this function and the size
computation loop above could drop their `STRLEN()` calls. That touches other
callers, so it does not belong in this PR.
--
Reply to this email directly or view it on GitHub:
https://github.com/vim/vim/pull/20925#issuecomment-5168219865
You are receiving this because you are subscribed to this thread.
Message ID: <vim/vim/pull/20925/[email protected]>
--
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
---
You received this message because you are subscribed to the Google Groups "vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To view this discussion visit https://groups.google.com/d/msgid/vim_dev/vim/vim/pull/20925/c5168219865%40github.com.
----==_mimepart_6a70b0544bc0f_f211802508eb
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div style=3D"display: flex; flex-wrap: wrap; white-space: pre-wrap; align-=
items: center; "><img height=3D"20" width=3D"20" style=3D"border-radius:50%=
; margin-right: 4px;" decoding=3D"async" src=3D"https://avatars.githubuserc=
ontent.com/u/518808" /><strong>h-east</strong> left a comment <a href=3D"ht=
tps://github.com/vim/vim/pull/20925#issuecomment-5168219865">(vim/vim#20925=
)</a></div>
<p dir=3D"auto">The change itself is correct. <code class=3D"notranslate">=
STRCAT()</code> rescans the result from the start<br>
for every appended character, so the loop was O(n^2) in the length of the<b=
r>
result, and keeping a tail pointer makes it linear. Moving the terminating=
<br>
NUL to after the loop is safe here because the loop has no early exit. The=
<br>
tests need work though.</p>
<h3 dir=3D"auto">The new test is not portable, Windows CI is red</h3>
<p dir=3D"auto">Three assertions fail on the Windows runners. <code class=
=3D"notranslate">strtrans("\x01\x1f\x7f")</code><br>
returns the 0x7f unchanged, and both cases expecting <code class=3D"notrans=
late"><9f></code> come back with a<br>
literal 0x9f.</p>
<p dir=3D"auto">The cause is 'isprint'. <code class=3D"notranslate">vim_is=
printc()</code> decides printability of anything<br>
below 0x100 from <code class=3D"notranslate">g_chartab</code>, which is bui=
lt from 'isprint', and the default<br>
differs per platform: <code class=3D"notranslate">"@,~-255"</code> on Win32=
and VMS, <code class=3D"notranslate">"@,161-255"</code> elsewhere.<br>
So 0x7f and 0x9f are printable on Windows and <code class=3D"notranslate">t=
ransstr()</code> correctly leaves<br>
them alone there.</p>
<p dir=3D"auto">Two ways out. Either set 'isprint' in the test so the expe=
ctations hold<br>
everywhere:</p>
<div class=3D"highlight highlight-source-viml" dir=3D"auto"><pre class=3D"n=
otranslate"> <span class=3D"pl-k">let</span> save_isprint <span class=3D"p=
l-k">=3D</span> &<span class=3D"pl-c1">isprint</span>
<span class=3D"pl-c1">set</span> <span class=3D"pl-c1">isprint</span><spa=
n class=3D"pl-k">=3D</span><span class=3D"pl-smi">@</span>,<span class=3D"p=
l-c1">161</span><span class=3D"pl-k">-</span><span class=3D"pl-c1">255</spa=
n>
<span class=3D"pl-k">...</span>
<span class=3D"pl-k">let</span> &<span class=3D"pl-c1">isprint</span>=
<span class=3D"pl-k">=3D</span> save_isprint</pre></div>
<p dir=3D"auto">Or drop the sub-0x100 cases and rely on the <code class=3D"=
notranslate">nr2char(0x200b)</code> and<br>
<code class=3D"notranslate">nr2char(0xfeff)</code> assertions, which go thr=
ough <code class=3D"notranslate">utf_printable()</code> and are not<br>
affected by 'isprint'. Those two already pass on every runner.</p>
<p dir=3D"auto">The first option is better if you want to keep covering the=
two-digit<br>
<code class=3D"notranslate">transchar_hex()</code> output, since <code clas=
s=3D"notranslate"><200b></code> and <code class=3D"notranslate"><f=
eff></code> only exercise the<br>
four-digit branch.</p>
<h3 dir=3D"auto">Test coverage does not include the non-multi-byte path</h3=
>
<p dir=3D"auto">The second commit says it covers all <code class=3D"notrans=
late">strtrans()</code> code paths, but every test<br>
runs with the default 'encoding'. <code class=3D"notranslate">transstr()</=
code> branches on <code class=3D"notranslate">has_mbyte</code> for<br>
the size computation, and the <code class=3D"notranslate">else</code> branc=
h using <code class=3D"notranslate">vim_strsize()</code> is never<br>
reached.</p>
<p dir=3D"auto"><code class=3D"notranslate">test_functions.vim</code> alrea=
dy switches encoding in several tests, so a block<br>
like this would close the gap, with 'isprint' pinned for the same reason as=
<br>
above:</p>
<div class=3D"highlight highlight-source-viml" dir=3D"auto"><pre class=3D"n=
otranslate"> <span class=3D"pl-k">let</span> save_isprint <span class=3D"p=
l-k">=3D</span> &<span class=3D"pl-c1">isprint</span>
<span class=3D"pl-c1">set</span> <span class=3D"pl-c1">encoding</span><sp=
an class=3D"pl-k">=3D</span>latin1
<span class=3D"pl-c1">set</span> <span class=3D"pl-c1">isprint</span><spa=
n class=3D"pl-k">=3D</span><span class=3D"pl-smi">@</span>,<span class=3D"p=
l-c1">161</span><span class=3D"pl-k">-</span><span class=3D"pl-c1">255</spa=
n>
<span class=3D"pl-c1">call</span> <span class=3D"pl-en">assert_equal</spa=
n>(<span class=3D"pl-s"><span class=3D"pl-pds">'</span>a^Mb<span class=3D"p=
l-pds">'</span></span>, <span class=3D"pl-en">strtrans</span>(<span class=
=3D"pl-s"><span class=3D"pl-pds">"</span>a<span class=3D"pl-cce">\r</span>b=
<span class=3D"pl-pds">"</span></span>))
<span class=3D"pl-k">...</span>
<span class=3D"pl-c1">set</span> <span class=3D"pl-c1">encoding</span><sp=
an class=3D"pl-k">=3D</span>utf<span class=3D"pl-k">-</span><span class=3D"=
pl-c1">8</span>
<span class=3D"pl-k">let</span> &<span class=3D"pl-c1">isprint</span>=
<span class=3D"pl-k">=3D</span> save_isprint</pre></div>
<p dir=3D"auto">Please verify the expected strings on your side, the point =
is the branch<br>
rather than the exact output.</p>
<h3 dir=3D"auto">Prefer the pattern already used in this file</h3>
<p dir=3D"auto">For the single-byte case the patch does <code class=3D"notr=
anslate">STRCPY()</code> followed by <code class=3D"notranslate">STRLEN(d)<=
/code>,<br>
which walks the translated character twice. <code class=3D"notranslate">tr=
ans_characters()</code> a few lines<br>
above solves the same problem by taking the length first:</p>
<div class=3D"highlight highlight-source-c" dir=3D"auto"><pre class=3D"notr=
anslate"> <span class=3D"pl-s1">trs</span> <span class=3D"pl-c1">=3D</s=
pan> <span class=3D"pl-en">transchar_byte</span>(<span class=3D"pl-c1">*</s=
pan><span class=3D"pl-s1">p</span><span class=3D"pl-c1">++</span>);
<span class=3D"pl-s1">trs_len</span> <span class=3D"pl-c1">=3D</span> =
(<span class=3D"pl-smi">int</span>)<span class=3D"pl-en">STRLEN</span>(<spa=
n class=3D"pl-s1">trs</span>);
<span class=3D"pl-en">mch_memmove</span>(<span class=3D"pl-s1">d</span=
>, <span class=3D"pl-s1">trs</span>, (<span class=3D"pl-smi">size_t</span>)=
<span class=3D"pl-s1">trs_len</span>);
<span class=3D"pl-s1">d</span> <span class=3D"pl-c1">+=3D</span> <span=
class=3D"pl-s1">trs_len</span>;</pre></div>
<p dir=3D"auto">That is one scan instead of two and matches the surrounding=
code. The strings<br>
are short so this is not about speed, it is about staying consistent with t=
he<br>
file.</p>
<h3 dir=3D"auto">Please add a measurement</h3>
<p dir=3D"auto">The commit message says this could impact <code class=3D"no=
translate">get_foldtext()</code> in <code class=3D"notranslate">fold.c</cod=
e>. The<br>
complexity argument is sound on its own, but a before/after timing on a lon=
g<br>
line would make the case concrete and would tell us whether this is a real<=
br>
user-visible win or a theoretical one. Something like <code class=3D"notra=
nslate">strtrans()</code> on a<br>
100000 byte line with a mix of control characters is enough.</p>
<h3 dir=3D"auto">Out of scope, but worth noting</h3>
<p dir=3D"auto"><code class=3D"notranslate">transchar_hex()</code> already =
knows how many bytes it wrote in its local <code class=3D"notranslate">i</c=
ode>, and<br>
it returns void. If it returned that length, both this function and the si=
ze<br>
computation loop above could drop their <code class=3D"notranslate">STRLEN(=
)</code> calls. That touches other<br>
callers, so it does not belong in this PR.</p>
<p style=3D"font-size:small;-webkit-text-size-adjust:none;color:#666;">&mda=
sh;<br />Reply to this email directly, <a href=3D"https://github.com/vim/vi=
m/pull/20925#issuecomment-5168219865">view it on GitHub</a>, or <a href=3D"=
https://github.com/notifications/unsubscribe-auth/ACY5DGAJDBX62LKFHCKBSPL5I=
CT5JAVCNFSNUABEKJSXA33TNF2G64TZHM2DAOJZG42DQMR3JFZXG5LFHM2TANBWG42DQMBRGGQX=
MAQ">unsubscribe</a>.<br />Triage notifications, keep track of coding agent=
tasks and review pull requests on the go with GitHub Mobile for <a href=3D=
"https://github.com/notifications/mobile/ios/ACY5DGATQCFCNHQV7KUHESL5ICT5JA=
5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMJWHAZDCOJYGY22M4TFM=
FZW63VKON2WE43DOJUWEZLEUVSXMZLOOSVGM33PORSXEX3JN5ZQ">iOS</a> and <a href=3D=
"https://github.com/notifications/mobile/android/ACY5DGCZQRIKLK2A65YNRBD5IC=
T5JA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMJWHAZDCOJYGY22M=
4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSXGM33PORSXEX3BNZSHE33JMQ">Android</a>.=
Download it today!
<br />You are receiving this because you are subscribed to this thread.<img=
src=3D"https://github.com/notifications/beacon/ACY5DGBL4A2XXMK4RZYYCKL5ICT=
5JBFCNFSM6AAAAAC4X4CQ6GWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3=
TUL5UWJTYAAAAACNAMY3M2M4TFMFZW63VKON2WE43DOJUWEZLE.gif" height=3D"1" width=
=3D"1" alt=3D"" /><span style=3D"color: transparent; font-size: 0; display:=
none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: =
0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><vim/vi=
m/pull/20925/c5168219865</span><span>@</span><span>github</span><span>.</sp=
an><span>com></span></span></p>
<script type=3D"application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/vim/vim/pull/20925#issuecomment-5168219865",
"url": "https://github.com/vim/vim/pull/20925#issuecomment-5168219865",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>
<p></p>
-- <br />
-- <br />
You received this message from the "vim_dev" maillist.<br />
Do not top-post! Type your reply below the text you are replying to.<br />
For more information, visit <a href=3D"http://www.vim.org/maillist.php">htt=
p://www.vim.org/maillist.php</a><br />
<br />
--- <br />
You received this message because you are subscribed to the Google Groups &=
quot;vim_dev" group.<br />
To unsubscribe from this group and stop receiving emails from it, send an e=
mail to <a href=3D"mailto:[email protected]">vim_dev+uns=
[email protected]</a>.<br />
To view this discussion visit <a href=3D"https://groups.google.com/d/msgid/=
vim_dev/vim/vim/pull/20925/c5168219865%40github.com?utm_medium=3Demail&utm_=
source=3Dfooter">https://groups.google.com/d/msgid/vim_dev/vim/vim/pull/209=
25/c5168219865%40github.com</a>.<br />
----==_mimepart_6a70b0544bc0f_f211802508eb--