From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs,gmane.emacs.devel Subject: bug#55719: [PATCH] bindat strz fixes Date: Tue, 31 May 2022 19:00:50 -0400 Message-ID: References: <6b1670d3-ae69-7f95-0e7d-d7cee0763c4a@rhansen.org> Reply-To: Stefan Monnier Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="36353"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux) Cc: 55719@debbugs.gnu.org, emacs-devel@gnu.org To: Richard Hansen Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Wed Jun 01 01:02:42 2022 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nwAss-0009HP-6h for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 01 Jun 2022 01:02:42 +0200 Original-Received: from localhost ([::1]:40486 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1nwAsq-0003Y1-Ki for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 31 May 2022 19:02:40 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:42924) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nwAsF-0003Ww-N3 for bug-gnu-emacs@gnu.org; Tue, 31 May 2022 19:02:04 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:55218) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1nwAsE-00077X-93 for bug-gnu-emacs@gnu.org; Tue, 31 May 2022 19:02:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1nwAsE-0005TQ-6n for bug-gnu-emacs@gnu.org; Tue, 31 May 2022 19:02:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 31 May 2022 23:02:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 55719 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 55719-submit@debbugs.gnu.org id=B55719.165403806220953 (code B ref 55719); Tue, 31 May 2022 23:02:02 +0000 Original-Received: (at 55719) by debbugs.gnu.org; 31 May 2022 23:01:02 +0000 Original-Received: from localhost ([127.0.0.1]:49115 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1nwArF-0005Rn-Te for submit@debbugs.gnu.org; Tue, 31 May 2022 19:01:02 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:31563) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1nwArE-0005RI-7U for 55719@debbugs.gnu.org; Tue, 31 May 2022 19:01:00 -0400 Original-Received: from pmg1.iro.umontreal.ca (localhost.localdomain [127.0.0.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id 968851008D2; Tue, 31 May 2022 19:00:54 -0400 (EDT) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id 44D7C1006B0; Tue, 31 May 2022 19:00:52 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1654038052; bh=L8WzC54nmJpMBmXQzpQPva/z3+L+ZNmJ/8mziQlajfE=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=nc+243mt2HPBGHIqL2lflXM+Oe+ux0sp7uR9DWj469ZQwCdUxLMTUdIZVQFduAHXQ gcw7I5lKBzD8ZzzrfNSTFu2IKUUyocQvsN3xH/Cg3vB+6bTtDffm3v7MaAsRB65t7Z bxa+1ZxahCW+wSyoChGp0uTgAQUZpGjuAwh1ZrYPK16dUum7DI7X5VVFMO4M0yDqGu geKzagal+PNtQ9OjlBC6UC3xVkoDlsNaxyb2CdRfDtcuvNvi3yCg97us1hiwFUx9VV 3G787vyuNjId2ICjPhOP64Wk2WZtQPYSHdmOrqfmOpHdUCZiU1WteB/kCviF+/vMdf d5SjUORzcVu1Q== Original-Received: from pastel (unknown [45.72.221.51]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 0EC55120445; Tue, 31 May 2022 19:00:52 -0400 (EDT) In-Reply-To: <6b1670d3-ae69-7f95-0e7d-d7cee0763c4a@rhansen.org> (Richard Hansen's message of "Mon, 30 May 2022 12:53:31 -0400") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:233469 gmane.emacs.devel:290454 Archived-At: > 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