Re: [PATCH] bindat strz fixes

[Stefan Monnier]

> diff --git a/test/lisp/emacs-lisp/bindat-tests.el 
> b/test/lisp/emacs-lisp/bindat-tests.el
> index 7722cf6c02..53c0c359d8 100644
> --- a/test/lisp/emacs-lisp/bindat-tests.el
> +++ b/test/lisp/emacs-lisp/bindat-tests.el
> @@ -162,4 +162,64 @@ bindat-test--recursive
>                                          (bindat-pack bindat-test--LEB128 n))
>                           n)))))))
>  
> +(let ((spec (bindat-type :pack-var v
> +                         (x strz 2 :pack-val v)
> +                         :unpack-val x)))

Any particular reason why you define it this way instead of just

    (bindat-type strz 2)

?

> +  (ert-deftest bindat-test--strz-fixedlen-len ()
> +    (should (equal (bindat-length spec "") 2))
> +    (should (equal (bindat-length spec "a") 2)))
> +
> +  (ert-deftest bindat-test--strz-fixedlen-len-overflow ()
> +    (should (equal (bindat-length spec "abc") 2)))
> +
> +  (ert-deftest bindat-test--strz-fixedlen-pack ()
> +    (should (equal (bindat-pack spec "") "\0\0"))
> +    (should (equal (bindat-pack spec "a") "\141\0")))

LGTM.

> +  (ert-deftest bindat-test--strz-fixedlen-pack-overflow ()
> +    :expected-result :failed
> +    (should (equal (bindat-pack spec "abc") "\141\0")))

I think this changes the intended semantics.  Until now `strz N` has
meant that N bytes are used to encode the string and that it can
hold upto a string of length N (in which case there's no terminating NUL
byte).  I agree that it's not the only valid semantics, but I'm not sure
we want to change it at this point.

Do you have a particular reason to make this change.

> +  (ert-deftest bindat-test--strz-fixedlen-unpack ()
> +    (should (equal (bindat-unpack spec "\0\0") ""))
> +    (should (equal (bindat-unpack spec "a\0") "a"))))

How 'bout

     (bindat-unpack spec "ab")

?

> +(let ((spec (bindat-type :pack-var v
> +                         (x strz :pack-val v)
> +                         :unpack-val x)))

Similarly here, I'd use just (bindat-type strz)

> +  (ert-deftest bindat-test--strz-varlen-len ()
> +    :expected-result :failed
> +    (should (equal (bindat-length spec "") 1))
> +    (should (equal (bindat-length spec "abc") 4)))
> +
> +  (ert-deftest bindat-test--strz-varlen-pack ()
> +    :expected-result :failed
> +    (should (equal (bindat-pack spec "") "\0"))
> +    (should (equal (bindat-pack spec "abc") "\141\142\143\0")))
> +
> +  (ert-deftest bindat-test--strz-varlen-unpack ()
> +    :expected-result :failed
> +    (should (equal (bindat-unpack spec "\0") ""))
> +    (should (equal (bindat-unpack spec "\141\142\143\0") "abc"))))

Looks good (tho I'd write "abc\0" i.s.o "\141\142\143\0").
Not sure what we should do about (bindat-unpack spec "abc")?

> diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
> index c6d64975ec..f66458296a 100644
> --- a/lisp/emacs-lisp/bindat.el
> +++ b/lisp/emacs-lisp/bindat.el
> @@ -687,10 +687,9 @@ bindat--type
>    (bindat--pcase op
>      ('unpack `(bindat--unpack-strz ,len))
>      (`(length ,val)
> -     `(cl-incf bindat-idx ,(cond
> -                            ((null len) `(length ,val))
> -                            ((numberp len) len)
> -                            (t `(or ,len (length ,val))))))
> +     `(cl-incf bindat-idx ,(if (numberp len)
> +                               len
> +                             `(1+ (length ,val)))))

`len` is supposed to be an ELisp *expression*.  E.g. it can be

    (+ a 4)

in which case (numberp len) will fail yet we should return the value of
`len` rather than (1+ (length ,val)).  In the original code, the cases
for (null len) and (numberp len) are *optimizations*.

I haven't yet looked at the rest of the patches.  If you can update your
patches based on this feedback, that would be great, but in the worst
case, I'll get to reviewing the rest sooner or later anyway.


        Stefan