New implementation of mb_strimwidth

This new implementation of mb_strimwidth uses the new text
encoding conversion filters. Changes from the previous
implementation:

• mb_strimwidth allows a negative 'from' argument, which
should count backwards from the end of the string. However,
the implementation of this feature was buggy (starting right
from when it was first implemented).

It used the following code:

    if ((from < 0) || (width < 0)) {
        swidth = mbfl_strwidth(&string);
    }
    if (from < 0) {
        from += swidth;
    }

Do you see the bug? 'from' is a count of CODEPOINTS, but
'swidth' is a count of TERMINAL COLUMNS. Adding those two
together does not make sense. If there were no fullwidth
characters in the input string, then the two counts coincide
and the feature would work correctly. However, each
fullwidth character would throw the result off by one,
causing more characters to be skipped than was requested.

• mb_strimwidth also allows a negative 'width' argument,
which again counts backwards from the end of the string;
in this case, it is not determining the START of the portion
which we want to extract, but rather, the END of that portion.
Perhaps unsurprisingly, this feature was also buggy.

Code:

    if (width < 0) {
        width = swidth + width - from;
    }

'swidth + width' is fine here; the problem is '- from'.
Again, that is subtracting a count of CODEPOINTS from a
count of TERMINAL COLUMNS. In this case, we really need
to count the terminal width of the string prefix skipped
over by 'from', and subtract that rather than the number
of codepoints which are being skipped.

As a result, if a 'from' count was passed along with a
negative 'width', for every fullwidth character in the
skipped prefix, the result of mb_strimwidth was one
terminal column wider than requested.

Since these situations were covered by unit tests, you
might wonder why the bugs were not caught. Well, as far as
I can see, it looks like the author of the 'tests' just
captured the actual output of mb_strimwidth and defined it
as 'correct'. The tests were written in such a way that it
was difficult to examine them and see whether they made
sense or not; but a careful examination of the inputs and
outputs clearly shows that the legacy tests did not conform
to the documented contract of mb_strimwidth.

• The old implementation would always pass the input string
through decoding/encoding filters before returning it to
the caller, even if it fit within the specified width. This
means that invalid byte sequences would be converted to
error markers. For performance, the new implementation
returns the very same string which was passed in if it
does not exceed the specified width. This means that
erroneous byte sequences are not converted to error markers
unless it is necessary to trim the string.

• The same applies to the 'trim marker' string.

• The old implementation was buggy in the (unusual)
case that the trim marker is wider than the requested
maximum width of the result. It did an unsigned subtraction
of the requested width and the width of the trim marker. If the
width of the trim marker was greater, that subtraction would
underflow and yield a huge number. As a result, mb_strimwidth
would then pass the input string through, even if it was
far wider than the requested maximum width.

In that case, since the input string is wider than the
requested width, and NONE of it will fit together with the
trim marker, the new implementation returns just the trim
marker. This is the one case where the output can be wider
than the requested width: when BOTH the input string and
also the trim marker are too wide.

• Since it passed the input string and trim marker through
decoding/encoding filters, when using "Quoted-Printable" as
the encoding, newlines could be inserted into the trim marker
to maintain the maximum line length for QP.

This is an extremely bizarre use case and I don't think there
is any point in worrying about it. QP will be removed from
mbstring in time, anyways.

PERFORMANCE:

• From micro-benchmarking with various input string lengths and
text encodings, it appears that the new implementation is 2-3x
faster for UTF-8 and UTF-16. For legacy Japanese text encodings
like ISO-2022-JP or SJIS, the new implementation is perhaps 25%
faster.

• Note that correctly implementing negative 'from' and 'width'
arguments imposes a small performance burden in such cases; one
which the old implementation did not pay. This slightly skews
benchmarking results in favor of the old implementation. However,
even so, the new implementation is faster in all cases which I
tested.

Author: alexdowad

ext/mbstring/libmbfl/mbfl/mbfilter.c

@@ -93,7 +93,6 @@
 #include "filters/mbfilter_singlebyte.h"
 #include "filters/mbfilter_utf8.h"
 
-#include "eaw_table.h"
 #include "rare_cp_bitvec.h"
 
 /*
@@ -1191,205 +1190,6 @@ mbfl_strcut(
 	return result;
 }
 
-/* Some East Asian characters, when printed at a terminal (or the like), require double
- * the usual amount of horizontal space. We call these "fullwidth" characters. */
-static size_t character_width(unsigned int c)
-{
-	if (c < FIRST_DOUBLEWIDTH_CODEPOINT) {
-		return 1;
-	}
-
-	/* Do a binary search to see if we fall in any of the fullwidth ranges */
-	int lo = 0, hi = sizeof(mbfl_eaw_table) / sizeof(mbfl_eaw_table[0]);
-	while (lo < hi) {
-		int probe = (lo + hi) / 2;
-		if (c < mbfl_eaw_table[probe].begin) {
-			hi = probe;
-		} else if (c > mbfl_eaw_table[probe].end) {
-			lo = probe + 1;
-		} else {
-			return 2;
-		}
-	}
-
-	return 1;
-}
-
-size_t mbfl_strwidth(mbfl_string *string)
-{
-	if (!string->len) {
-		return 0;
-	}
-
-	size_t width = 0;
-	uint32_t wchar_buf[128];
-	unsigned char *in = string->val;
-	size_t in_len = string->len;
-	unsigned int state = 0;
-
-	while (in_len) {
-		size_t out_len = string->encoding->to_wchar(&in, &in_len, wchar_buf, 128, &state);
-		while (out_len) {
-			/* NOTE: 'bad input' marker will be counted as 1 unit of width
-			 * If text conversion is performed with an ordinary ASCII character as
-			 * the 'replacement character', this will give us the correct display width. */
-			width += character_width(wchar_buf[--out_len]);
-		}
-	}
-
-	return width;
-}
-
-
-/*
- *  strimwidth
- */
-struct collector_strimwidth_data {
-	mbfl_convert_filter *decoder;
-	mbfl_convert_filter *decoder_backup;
-	mbfl_memory_device device;
-	size_t from;
-	size_t width;
-	size_t outwidth;
-	size_t outchar;
-	size_t endpos;
-	int status;
-};
-
-static int
-collector_strimwidth(int c, void* data)
-{
-	struct collector_strimwidth_data *pc = (struct collector_strimwidth_data*)data;
-
-	switch (pc->status) {
-	case 10:
-		(*pc->decoder->filter_function)(c, pc->decoder);
-		break;
-	default:
-		if (pc->outchar >= pc->from) {
-			pc->outwidth += character_width(c);
-
-			if (pc->outwidth > pc->width) {
-				if (pc->status == 0) {
-					pc->endpos = pc->device.pos;
-					mbfl_convert_filter_copy(pc->decoder, pc->decoder_backup);
-				}
-				pc->status++;
-				(*pc->decoder->filter_function)(c, pc->decoder);
-				pc->outchar++;
-				return -1;
-			} else {
-				(*pc->decoder->filter_function)(c, pc->decoder);
-			}
-		}
-		pc->outchar++;
-		break;
-	}
-
-	return 0;
-}
-
-mbfl_string *
-mbfl_strimwidth(
-    mbfl_string *string,
-    mbfl_string *marker,
-    mbfl_string *result,
-    size_t from,
-    size_t width)
-{
-	struct collector_strimwidth_data pc;
-	mbfl_convert_filter *encoder;
-	size_t n, mkwidth;
-	unsigned char *p;
-
-	mbfl_string_init(result);
-	result->encoding = string->encoding;
-	mbfl_memory_device_init(&pc.device, MIN(string->len, width), 0);
-
-	/* output code filter */
-	pc.decoder = mbfl_convert_filter_new(
-	    &mbfl_encoding_wchar,
-	    string->encoding,
-	    mbfl_memory_device_output, 0, &pc.device);
-	pc.decoder_backup = mbfl_convert_filter_new(
-	    &mbfl_encoding_wchar,
-	    string->encoding,
-	    mbfl_memory_device_output, 0, &pc.device);
-	/* wchar filter */
-	encoder = mbfl_convert_filter_new(
-	    string->encoding,
-	    &mbfl_encoding_wchar,
-	    collector_strimwidth, 0, &pc);
-	if (pc.decoder == NULL || pc.decoder_backup == NULL || encoder == NULL) {
-		mbfl_convert_filter_delete(encoder);
-		mbfl_convert_filter_delete(pc.decoder);
-		mbfl_convert_filter_delete(pc.decoder_backup);
-		return NULL;
-	}
-	mkwidth = 0;
-	if (marker) {
-		mkwidth = mbfl_strwidth(marker);
-	}
-	pc.from = from;
-	pc.width = width - mkwidth;
-	pc.outwidth = 0;
-	pc.outchar = 0;
-	pc.status = 0;
-	pc.endpos = 0;
-
-	/* feed data */
-	p = string->val;
-	n = string->len;
-	if (p != NULL) {
-		while (n > 0) {
-			n--;
-			if ((*encoder->filter_function)(*p++, encoder) < 0) {
-				break;
-			}
-		}
-		mbfl_convert_filter_flush(encoder);
-		if (pc.status != 0 && mkwidth > 0) {
-			pc.width += mkwidth;
-			if (n > 0) {
-				while (n > 0) {
-					if ((*encoder->filter_function)(*p++, encoder) < 0) {
-						break;
-					}
-					n--;
-				}
-				mbfl_convert_filter_flush(encoder);
-			} else if (pc.outwidth > pc.width) {
-				pc.status++;
-			}
-			if (pc.status != 1) {
-				pc.status = 10;
-				pc.device.pos = pc.endpos;
-				mbfl_convert_filter_copy(pc.decoder_backup, pc.decoder);
-				mbfl_convert_filter_reset(encoder, marker->encoding, &mbfl_encoding_wchar);
-				p = marker->val;
-				n = marker->len;
-				while (n > 0) {
-					if ((*encoder->filter_function)(*p++, encoder) < 0) {
-						break;
-					}
-					n--;
-				}
-				mbfl_convert_filter_flush(encoder);
-			}
-		} else if (pc.status != 0) {
-			pc.device.pos = pc.endpos;
-			mbfl_convert_filter_copy(pc.decoder_backup, pc.decoder);
-		}
-		mbfl_convert_filter_flush(pc.decoder);
-	}
-	result = mbfl_memory_device_result(&pc.device, result);
-	mbfl_convert_filter_delete(encoder);
-	mbfl_convert_filter_delete(pc.decoder);
-	mbfl_convert_filter_delete(pc.decoder_backup);
-
-	return result;
-}
-
 
 /*
  *  MIME header encode

ext/mbstring/libmbfl/mbfl/mbfilter.h

@@ -231,18 +231,6 @@ mbfl_substr(mbfl_string *string, mbfl_string *result, size_t from, size_t length
 MBFLAPI extern mbfl_string *
 mbfl_strcut(mbfl_string *string, mbfl_string *result, size_t from, size_t length);
 
-/*
- *  strwidth
- */
-MBFLAPI extern size_t
-mbfl_strwidth(mbfl_string *string);
-
-/*
- *  strimwidth
- */
-MBFLAPI extern mbfl_string *
-mbfl_strimwidth(mbfl_string *string, mbfl_string *marker, mbfl_string *result, size_t from, size_t width);
-
 /*
  * MIME header encode
  */