From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Damien Cassou Newsgroups: gmane.emacs.bugs Subject: bug#65790: 29.1; "docstring wider than 80 characters" when there is no docstring Date: Tue, 12 Sep 2023 21:22:26 +0200 Message-ID: <87bke7nqml.fsf@cassou.me> References: <87edjbp1aq.fsf@cassou.me> <87fs3qwcoi.fsf@cassou.me> <87o7iaikxd.fsf@cassou.me> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="23410"; mail-complaints-to="usenet@ciao.gmane.io" To: Stefan Kangas , 65790@debbugs.gnu.org Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Sep 12 21:23:13 2023 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 1qg8yf-0005rv-C6 for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 12 Sep 2023 21:23:13 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qg8yR-0005Ov-5B; Tue, 12 Sep 2023 15:22:59 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qg8yP-0005OX-GC for bug-gnu-emacs@gnu.org; Tue, 12 Sep 2023 15:22:57 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qg8yP-00013v-8a for bug-gnu-emacs@gnu.org; Tue, 12 Sep 2023 15:22:57 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qg8yU-0006Q2-15 for bug-gnu-emacs@gnu.org; Tue, 12 Sep 2023 15:23:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Damien Cassou Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 12 Sep 2023 19:23:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 65790 X-GNU-PR-Package: emacs Original-Received: via spool by 65790-submit@debbugs.gnu.org id=B65790.169454656124645 (code B ref 65790); Tue, 12 Sep 2023 19:23:01 +0000 Original-Received: (at 65790) by debbugs.gnu.org; 12 Sep 2023 19:22:41 +0000 Original-Received: from localhost ([127.0.0.1]:60102 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qg8y8-0006PQ-9o for submit@debbugs.gnu.org; Tue, 12 Sep 2023 15:22:41 -0400 Original-Received: from mail.choca.pics ([2001:910:1410:500::1]:37344) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qg8y4-0006PD-VG for 65790@debbugs.gnu.org; Tue, 12 Sep 2023 15:22:38 -0400 Original-Received: from localhost (localhost.localdomain [IPv6:::1]) by mail.choca.pics (Postfix) with ESMTP id AAD0D18192DD8; Tue, 12 Sep 2023 21:22:28 +0200 (CEST) Original-Received: from mail.choca.pics ([IPv6:::1]) by localhost (mail.choca.pics [IPv6:::1]) (amavis, port 10032) with ESMTP id pOX2uMwHNTB8; Tue, 12 Sep 2023 21:22:27 +0200 (CEST) Original-Received: from localhost (localhost.localdomain [IPv6:::1]) by mail.choca.pics (Postfix) with ESMTP id 7F21B18192DF7; Tue, 12 Sep 2023 21:22:27 +0200 (CEST) X-Virus-Scanned: amavis at choca.pics Original-Received: from mail.choca.pics ([IPv6:::1]) by localhost (mail.choca.pics [IPv6:::1]) (amavis, port 10026) with ESMTP id 8KMej7ZQcPYo; Tue, 12 Sep 2023 21:22:27 +0200 (CEST) Original-Received: from localhost (91.60.75.86.rev.sfr.net [86.75.60.91]) by mail.choca.pics (Postfix) with ESMTPSA id F3FEC18192DD8; Tue, 12 Sep 2023 21:22:26 +0200 (CEST) In-Reply-To: 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-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:270215 Archived-At: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Thank you for your feedback. 3 patch files are attached to this email. My answers below: Stefan Kangas writes: > Damien Cassou writes: > I think this also could use some tests, see bytecomp-tests.el:964 and > onwards. I've added tests to bytecomp-tests.el as suggested. (why not cl-macs-tests.el instead?) >> * lisp/emacs-lisp/cl-macs.el (cl-defsubst): Reduce likelihood of >> "docstring wider than 80 characters" errors in generated code. > > Please remember to include the bug number when it is known. done >> - ,(format "compiler-macro for inlining `%s'." name) >> [=E2=80=A6] >> + ,(internal--format-docstring-line "compiler-macro for `%s'= ." name) > > Why drop the word "inlining"? I felt this word was not really important because (1) this is generated code and (2) the docstring starts with "compiler-macro". Removing it reduces the likelihood of the error message. I added an explanation in the commit message. If the word is important I can add it back. > (This comment might need reflowing.) done > Could we keep the old format when possible, and use the new one only > when needed? I tried to do that, is that what you want? I feel the added complexity isn't worth it so I'm fine changing it back to the simpler version if you change your mind. --=20 Damien Cassou "Success is the ability to go from one failure to another without losing enthusiasm." --Winston Churchill --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0001-bytecomp-tests.el-Add-new-helper-function.patch >From 28c90e4c3371501f2965ff5aac0d5108572fa674 Mon Sep 17 00:00:00 2001 From: Damien Cassou Date: Mon, 11 Sep 2023 21:59:45 +0200 Subject: [PATCH 1/3] bytecomp-tests.el: Add new helper function * test/lisp/emacs-lisp/bytecomp-tests.el (bytecomp--without-warning-test): Add helper function. (bytecomp-warn--ignore): Use the helper. --- test/lisp/emacs-lisp/bytecomp-tests.el | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el index cd1bda3ec55..c0adab75269 100644 --- a/test/lisp/emacs-lisp/bytecomp-tests.el +++ b/test/lisp/emacs-lisp/bytecomp-tests.el @@ -937,14 +937,17 @@ bytecomp--with-warning-test (should (re-search-forward (string-replace " " "[ \n]+" re-warning))))))) +(defun bytecomp--without-warning-test (form) + (bytecomp--with-warning-test "\\`\\'" form)) + (ert-deftest bytecomp-warn--ignore () (bytecomp--with-warning-test "unused" '(lambda (y) 6)) - (bytecomp--with-warning-test "\\`\\'" ;No warning! + (bytecomp--without-warning-test '(lambda (y) (ignore y) 6)) (bytecomp--with-warning-test "assq" '(lambda (x y) (progn (assq x y) 5))) - (bytecomp--with-warning-test "\\`\\'" ;No warning! + (bytecomp--without-warning-test '(lambda (x y) (progn (ignore (assq x y)) 5)))) (ert-deftest bytecomp-warn-wrong-args () -- 2.41.0 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0002-Shorten-docstrings-generated-by-cl-defsubst-bug-6579.patch >From 4be1620fe344ebacd707bfb6c69d34eaa1a27df5 Mon Sep 17 00:00:00 2001 From: Damien Cassou Date: Tue, 12 Sep 2023 08:28:37 +0200 Subject: [PATCH 2/3] Shorten docstrings generated by cl-defsubst (bug#65790) * lisp/emacs-lisp/cl-macs.el (cl-defsubst): Remove the word "inlining" in the generated docstring as it is not very useful and increases the likelihood of "docstring wider than 80 characters" errors. Additionally, split the first line of the docstring into 2 lines if the function name is very long. --- lisp/emacs-lisp/cl-macs.el | 9 ++++++++- test/lisp/emacs-lisp/bytecomp-tests.el | 11 +++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el index c8e2610c8b0..dc17f992660 100644 --- a/lisp/emacs-lisp/cl-macs.el +++ b/lisp/emacs-lisp/cl-macs.el @@ -2931,7 +2931,14 @@ cl-defsubst ,(if (memq '&key args) `(&whole cl-whole &cl-quote ,@args) (cons '&cl-quote args)) - ,(format "compiler-macro for inlining `%s'." name) + ;; NB. This will produce incorrect results in some + ;; cases, as our coding conventions says that the first + ;; line must be a full sentence. However, if we don't + ;; word wrap we will have byte-compiler warnings about + ;; overly long docstrings. So we can't have a perfect + ;; result here, and choose to avoid the byte-compiler + ;; warnings. + ,(internal--format-docstring-line "compiler-macro for `%s'." name) (cl--defsubst-expand ',argns '(cl-block ,name ,@(cdr (macroexp-parse-body body))) nil diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el index c0adab75269..102616c9bb7 100644 --- a/test/lisp/emacs-lisp/bytecomp-tests.el +++ b/test/lisp/emacs-lisp/bytecomp-tests.el @@ -972,6 +972,17 @@ bytecomp-warn-wide-docstring/defvar (bytecomp--with-warning-test "defvar.*foo.*wider than.*characters" `(defvar foo t ,bytecomp-tests--docstring))) +(ert-deftest bytecomp-warn-wide-docstring/cl-defsubst () + (bytecomp--without-warning-test + `(cl-defsubst short-name () + "Do something.")) + (bytecomp--without-warning-test + `(cl-defsubst long-name-with-less-80-characters-but-still-quite-a-bit () + "Do something.")) + (bytecomp--with-warning-test "wider than.*characters" + `(cl-defsubst long-name-with-more-than-80-characters-yes-this-is-a-very-long-name-but-why-not!! () + "Do something."))) + (ert-deftest bytecomp-warn-quoted-condition () (bytecomp--with-warning-test "Warning: `condition-case' condition should not be quoted: 'arith-error" -- 2.41.0 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0003-Shorten-docstrings-generated-by-cl-defstruct-bug-657.patch >From 23e8316ef7659b361157c0659bdb96a4bc9cb1e7 Mon Sep 17 00:00:00 2001 From: Damien Cassou Date: Tue, 12 Sep 2023 08:28:37 +0200 Subject: [PATCH 3/3] Shorten docstrings generated by cl-defstruct (bug#65790) * lisp/emacs-lisp/cl-macs.el (cl-defstruct): Split the first line of generated docstrings if either the struct name or a field name is very long. This reduces the likelihood of "docstring wider than 80 characters" errors. --- lisp/emacs-lisp/cl-macs.el | 44 ++++++++++++++++++-------- test/lisp/emacs-lisp/bytecomp-tests.el | 17 ++++++++++ 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el index dc17f992660..ef00f9e4fe4 100644 --- a/lisp/emacs-lisp/cl-macs.el +++ b/lisp/emacs-lisp/cl-macs.el @@ -3187,18 +3187,27 @@ cl-defstruct ;; The arg "cl-x" is referenced by name in e.g. pred-form ;; and pred-check, so changing it is not straightforward. (push `(,defsym ,accessor (cl-x) - ,(concat - ;; NB. This will produce incorrect results - ;; in some cases, as our coding conventions - ;; says that the first line must be a full - ;; sentence. However, if we don't word wrap - ;; we will have byte-compiler warnings about - ;; overly long docstrings. So we can't have - ;; a perfect result here, and choose to avoid - ;; the byte-compiler warnings. - (internal--format-docstring-line - "Access slot \"%s\" of `%s' struct CL-X." slot name) - (if doc (concat "\n" doc) "")) + ,(let ((long-docstring + (format "Access slot \"%s\" of `%s' struct CL-X." slot name))) + (concat + ;; NB. This will produce incorrect results + ;; in some cases, as our coding conventions + ;; says that the first line must be a full + ;; sentence. However, if we don't word + ;; wrap we will have byte-compiler warnings + ;; about overly long docstrings. So we + ;; can't have a perfect result here, and + ;; choose to avoid the byte-compiler + ;; warnings. + (if (>= (length long-docstring) byte-compile-docstring-max-column) + (concat + (internal--format-docstring-line + "Access slot \"%s\" of CL-X." slot) + "\n" + (internal--format-docstring-line + "Struct CL-X is a `%s'." name)) + (internal--format-docstring-line long-docstring)) + (if doc (concat "\n" doc) ""))) (declare (side-effect-free t)) ,access-body) forms) @@ -3274,7 +3283,16 @@ cl-defstruct (push `(,cldefsym ,cname (&cl-defs (nil ,@descs) ,@args) ,(if (stringp doc) doc - (format "Constructor for objects of type `%s'." name)) + ;; NB. This will produce incorrect results in + ;; some cases, as our coding conventions says that + ;; the first line must be a full sentence. + ;; However, if we don't word wrap we will have + ;; byte-compiler warnings about overly long + ;; docstrings. So we can't have a perfect result + ;; here, and choose to avoid the byte-compiler + ;; warnings. + (internal--format-docstring-line + "Constructor for objects of type `%s'." name)) ,@(if (cl--safe-expr-p `(progn ,@(mapcar #'cl-second descs))) '((declare (side-effect-free t)))) (,con-fun ,@make)) diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el index 102616c9bb7..03aed5263b6 100644 --- a/test/lisp/emacs-lisp/bytecomp-tests.el +++ b/test/lisp/emacs-lisp/bytecomp-tests.el @@ -983,6 +983,23 @@ bytecomp-warn-wide-docstring/cl-defsubst `(cl-defsubst long-name-with-more-than-80-characters-yes-this-is-a-very-long-name-but-why-not!! () "Do something."))) +(ert-deftest bytecomp-warn-wide-docstring/cl-defstruct () + (bytecomp--without-warning-test + `(cl-defstruct short-name + field)) + (bytecomp--without-warning-test + `(cl-defstruct short-name + long-name-with-less-80-characters-but-still-quite-a-bit)) + (bytecomp--without-warning-test + `(cl-defstruct long-name-with-less-80-characters-but-still-quite-a-bit + field)) + (bytecomp--with-warning-test "wider than.*characters" + `(cl-defstruct short-name + long-name-with-more-than-80-characters-yes-this-is-a-very-long-name-but-why-not!!)) + (bytecomp--with-warning-test "wider than.*characters" + `(cl-defstruct long-name-with-more-than-80-characters-yes-this-is-a-very-long-name-but-why-not!! + field))) + (ert-deftest bytecomp-warn-quoted-condition () (bytecomp--with-warning-test "Warning: `condition-case' condition should not be quoted: 'arith-error" -- 2.41.0 --=-=-=--