Re: Replace XChar2b with unsigned in all font backends

[Alex Gramiak]

Andy Moreton <address@hidden> writes:

> On Mon 20 May 2019, Alex Gramiak wrote:
>
>> Andy Moreton <address@hidden> writes:
>>
>>> Confirmed. This patch replaces use of XChar2b (a 16bit type) with
>>> unsigned (usually 32bit) which seems wrong.
>>>
>>>     AndyM
>>
>> I originally used unsigned short for this patch, but Eli[1] nudged me
>> towards unsigned. Unsigned fits better with other parts of the font
>> system, e.g., *encode_char returns unsigned, and *text_extents takes a
>> pointer to unsigned.
>>
>> [1] https://lists.gnu.org/archive/html/emacs-devel/2019-05/msg00457.html
>
> Yes, but Eli was concerned with efficiency, but correctness comes first.
> If you are calling APIs that expect a pointer to an array of 16bit
> values, then an array of 32bit values will not suffice.

Right, at least not without converting the array; the xfont backend does
this when the API expects an array of 8bit values.

> Many of the comments around this code talk of 2-byte values, so
> changing the code to use 4-byte values is surprising to the reader.

I agree, but at least the char2b name signifies that it's a 2-byte
value.

> The changes also removed the explicit packing/unpacking of 16bit values,
> which may give rise to endianness issues on some systems.

I haven't dealt with endianness issues before, but from a quick search
[1][2] it doesn't appear that it's an issue:

[1] https://developer.ibm.com/articles/au-endianc/
[2] https://stackoverflow.com/a/7184905

> Please revert this patch to fix the build on master, and then revisit
> these changes after that.

I think that the solution is just to do a conversion on the w32font_draw
side. Can you confirm if the following diff fixes the build for you?

diff --git a/src/w32font.c b/src/w32font.c
index bd68e22cc9..d3ec2916a5 100644
--- a/src/w32font.c
+++ b/src/w32font.c
@@ -704,11 +704,20 @@ w32font_draw (struct glyph_string *s, int from, int to,
       int i;
 
       for (i = 0; i < len; i++)
-       ExtTextOutW (s->hdc, x + i, y, options, NULL,
-                    s->char2b + from + i, 1, NULL);
+        {
+          const wchar_t ch = s->char2b[from + i];
+          ExtTextOutW (s->hdc, x + i, y, options, NULL, &ch, 1, NULL);
+        }
     }
   else
-    ExtTextOutW (s->hdc, x, y, options, NULL, s->char2b + from, len, NULL);
+    {
+      USE_SAFE_ALLOCA;
+      wchar_t *str = SAFE_ALLOCA (len * (sizeof (*str)));
+      for (int i = 0; i < len; ++i)
+        str[i] = s->char2b[from + i];
+      ExtTextOutW (s->hdc, x, y, options, NULL, str, len, NULL);
+      SAFE_FREE ();
+    }
 
   /* Restore clip region.  */
   if (s->num_clips > 0)