bug#55897: [PATCH] bindat (str, strz): Convert to unibyte when packing

[Eli Zaretskii]

> Cc: monnier@iro.umontreal.ca
> Date: Sat, 11 Jun 2022 00:38:00 -0400
> From: Richard Hansen <rhansen@rhansen.org>
> 
>  (defun bindat--pack-str (len v)
> +  (if (multibyte-string-p v)
> +      (signal 'wrong-type-argument `(multibyte-string-p ,v)))

Isn't this too strict?  First, a string can be multibyte and
pure-ASCII:

  (let ((str (decode-coding-string "abcde" 'utf-8)))
    (multibyte-string-p str))
    => t

Shouldn't it be possible to use such strings here?

Furthermore, I think you said you wanted to extend bindat so it could
use multibyte string that contain ASCII and eight-bit characters?  If
so, this sounds like shooting ourselves in the foot?

>  (defun bindat--pack-str (len v)
> -  (if (multibyte-string-p v)
> -      (signal 'wrong-type-argument `(multibyte-string-p ,v)))
> -  (dotimes (i (min len (length v)))
> -    (aset bindat-raw (+ bindat-idx i) (aref v i)))
> -  (setq bindat-idx (+ bindat-idx len)))
> +  (let ((v (string-to-unibyte v)))
> +    (dotimes (i (min len (length v)))
> +      (aset bindat-raw (+ bindat-idx i) (aref v i)))
> +    (setq bindat-idx (+ bindat-idx len))))

And here you remove that error back?  Why does it make sense to
introduce an error message, only to remove it in the very next commit?
Please instead make a single change which incorporates both.

> --- a/test/lisp/emacs-lisp/bindat-tests.el
> +++ b/test/lisp/emacs-lisp/bindat-tests.el
> @@ -193,13 +193,15 @@ bindat-test--str-strz-multibyte
>    (dolist (spec (list (bindat-type str 2)
>                        (bindat-type strz 2)
>                        (bindat-type strz)))
> -    (should-error (bindat-pack spec (string-to-multibyte "x")))
> -    (should-error (bindat-pack spec (string-to-multibyte "\xff")))
> +    (should (equal (bindat-pack spec (string-to-multibyte "x")) "x\0"))
> +    (should (equal (bindat-pack spec (string-to-multibyte "\xff")) "\xff\0"))
>      (should-error (bindat-pack spec "💩"))
>      (should-error (bindat-pack spec "\N{U+ff}")))
>    (dolist (spec (list '((x str 2)) '((x strz 2))))
> -    (should-error (bindat-pack spec `((x . ,(string-to-multibyte "x")))))
> -    (should-error (bindat-pack spec `((x . ,(string-to-multibyte "\xff")))))
> +    (should (equal (bindat-pack spec `((x . ,(string-to-multibyte "x"))))
> +                   "x\0"))
> +    (should (equal (bindat-pack spec `((x . ,(string-to-multibyte "\xff"))))
> +                   "\xff\0"))
>      (should-error (bindat-pack spec '((x . "💩"))))
>      (should-error (bindat-pack spec '((x . "\N{U+ff}"))))))

Likewise here.

Thanks.

P.S. Please also mention the bug number in the log message of the next
version of the patch, since the number is now known.