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

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Richard Hansen <rhansen <at> rhansen.org>
Cc: 55897 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: bug#55897: [PATCH] bindat (str, strz): Convert to unibyte when packing
Date: Sat, 11 Jun 2022 11:11:15 +0300

> Cc: monnier <at> iro.umontreal.ca
> Date: Sat, 11 Jun 2022 00:38:00 -0400
> From: Richard Hansen <rhansen <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.