Towards a cleaner build

Towards a cleaner build

[Lars Ingebrigtsen]

I've got a cold, so I thought that it might be fun to have a look at
some of the byte-compile warnings to see whether there's any low-hanging
fruit that could be easily fixed.  Because warnings are an annoyance
when building Emacs, and hide real problems.

But, of course, I have questions:

In Info-edit:
info.el:4415:4:Warning: ‘Info-edit-mode’ is an obsolete function (as of 24.4);
    editing Info nodes by hand is not recommended.

which is

(make-obsolete 'Info-edit-mode
	       "editing Info nodes by hand is not recommended." "24.4")

(defun Info-edit ()
  "Edit the contents of this Info node."
  (interactive)
  (Info-edit-mode)
  (message "%s" (substitute-command-keys
		 "Editing: Type \\<Info-edit-mode-map>\\[Info-cease-edit] to return to info")))

(put 'Info-edit 'disabled "Editing Info nodes by hand is not recommended.
This feature will be removed in future.")

(make-obsolete 'Info-edit
	       "editing Info nodes by hand is not recommended." "24.4")


etc etc etc.  All the Info-edit stuff was declared obsolete in 24.4, and
I seem to recall that our policy for removing obsolete stuff is two
release cycles?  (I couldn't find anything in admin/notes that said
anything about this explicitly...)  So it OK to remove all the Info-edit
modes and commands now?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

In pcomplete/tar:
pcmpl-gnu.el:162:47:Warning: ‘pcomplete-suffix-list’ is an obsolete variable
    (as of 24.1).

But pcomplete-suffix-alist is used several times in the pcomplete source
code, like in pcomplete.el:

      (when (and (not (memq (char-before) pcomplete-suffix-list))
		 addsuffix)
	(insert-and-inherit pcomplete-termination-string)

And in pcomplete/tar, there's stuff like:

    (let ((pcomplete-suffix-list (if (boundp 'pcomplete-suffix-list)
                                     (cons ?= pcomplete-suffix-list))))

which seems...  odd...  Is this part of a rewrite that was never
finished?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

> pcmpl-gnu.el:162:47:Warning: ‘pcomplete-suffix-list’ is an obsolete variable
>     (as of 24.1).
>
> But pcomplete-suffix-alist is used several times in the pcomplete source
> code, like in pcomplete.el:
>
>       (when (and (not (memq (char-before) pcomplete-suffix-list))
> 		 addsuffix)
> 	(insert-and-inherit pcomplete-termination-string)
>
> And in pcomplete/tar, there's stuff like:
>
>     (let ((pcomplete-suffix-list (if (boundp 'pcomplete-suffix-list)
>                                      (cons ?= pcomplete-suffix-list))))
>
> which seems...  odd...  Is this part of a rewrite that was never
> finished?

Hmm... it's used, fundamentally, in pcomplete-insert-entry when the
third arg is non-nil.  There are two such cases, one of which can be
removed with the patch below, and the remaining one is in `pcomplete`
itself, which has been superseded by pcomplete-completions-at-point but
hasn't been declared obsolete (yet?) indeed.
Hmm...


        Stefan
        

diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index e080074927..8477733354 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -601,10 +601,10 @@ pcomplete-expand
 	       (> (length pcomplete-current-completions) 0)) ;??
       (delete-char (- pcomplete-last-completion-length))
       (while pcomplete-current-completions
-	(unless (pcomplete-insert-entry
-		 "" (car pcomplete-current-completions) t
-                 pcomplete-last-completion-raw)
-	  (insert-and-inherit pcomplete-termination-string))
+        (pcomplete-insert-entry
+	 "" (car pcomplete-current-completions) nil
+         pcomplete-last-completion-raw)
+	(insert-and-inherit pcomplete-termination-string)
 	(setq pcomplete-current-completions
 	      (cdr pcomplete-current-completions))))))
 

Re: Towards a cleaner build

[Lars Ingebrigtsen]

epa declares a bunch of functions for interactive use only, and then
uses the functions itself, which seems like an error, like this:

;;;###autoload
(defun epa-mail-verify ()
  "Verify OpenPGP cleartext signed messages in the current buffer.
The buffer is expected to contain a mail message."
  (declare (interactive-only t))
  (interactive)
  (epa-verify-cleartext-in-region (point-min) (point-max)))

`epa-verify-cleartext-in-region' is defined as interactive-only.

I suggest that the -in-region function have the interactive spec
removed.  Any objections?



In epa-verify-cleartext-in-region:
epa.el:998:46:Warning: ‘epa-verify-region’ is for interactive use only.
  ELC      epa-file.elc

In epa-mail-decrypt:
epa-mail.el:72:4:Warning: ‘epa-decrypt-armor-in-region’ is for interactive use
    only.

In epa-mail-verify:
epa-mail.el:80:4:Warning: ‘epa-verify-cleartext-in-region’ is for interactive
    use only.

In epa-mail-sign:
epa-mail.el:106:10:Warning: ‘epa-sign-region’ is for interactive use only.

In epa-mail-encrypt:
epa-mail.el:225:12:Warning: ‘epa-encrypt-region’ is for interactive use only.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: epa-mail

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> In epa-mail-decrypt:
> epa-mail.el:72:4:Warning: ‘epa-decrypt-armor-in-region’ is for interactive use
>     only.

[...]

> ;;;###autoload
> (defun epa-mail-verify ()
>   "Verify OpenPGP cleartext signed messages in the current buffer.
> The buffer is expected to contain a mail message."
>   (declare (interactive-only t))
>   (interactive)
>   (epa-verify-cleartext-in-region (point-min) (point-max)))
>
> `epa-verify-cleartext-in-region' is defined as interactive-only.
>
> I suggest that the -in-region function have the interactive spec
> removed.  Any objections?

Or!  Perhaps there should be no warning on calling an interactive-only
function from a function that's also marked as interactive-only?  These
are usually just shims around the former...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

minibuffer.el:2974:1:Warning: Unused lexical variable ‘p1’
minibuffer.el:2974:1:Warning: Unused lexical variable ‘p2’

(defun completion-pcm--optimize-pattern (p)

[...]

        (`(,(and p1 (pred symbolp)) ,(and p2 (guard (eq p1 p2))) . ,_)
         ;; Unused lexical variable warning due to body not using p1, p2.
         ;; https://debbugs.gnu.org/16771

And, indeed, the bug hasn't been fixed.

However, as far as I can tell, completion-pcm--optimize-pattern is never
called from anywhere in the source code, and it's an internal function.
Can it be just removed?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

> However, as far as I can tell, completion-pcm--optimize-pattern is never
> called from anywhere in the source code, and it's an internal function.
> Can it be just removed?

Fair enough.


        Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> However, as far as I can tell, completion-pcm--optimize-pattern is never
>> called from anywhere in the source code, and it's an internal function.
>> Can it be just removed?
>
> Fair enough.

OK; removing.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

In display-completion-list:
minibuffer.el:1741:12:Warning: display-completion-list called with 2
    arguments, but accepts only 1

But...

(defun display-completion-list (completions &optional common-substring)

*scratches head*

I wondered whether something else had declared that function to only
take one parameter, but no.  And I don't think the byte compiler
regularly outputs these warnings when calling a function recursively,
surely?  

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> In display-completion-list:
> minibuffer.el:1741:12:Warning: display-completion-list called with 2
>     arguments, but accepts only 1
>
> But...
>
> (defun display-completion-list (completions &optional common-substring)
>
> *scratches head*

    (defun display-completion-list (completions &optional common-substring)
      [...]
      (declare (advertised-calling-convention (completions) "24.4"))


-- Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> In display-completion-list:
>> minibuffer.el:1741:12:Warning: display-completion-list called with 2
>>     arguments, but accepts only 1
>>
>> But...
>>
>> (defun display-completion-list (completions &optional common-substring)
>>
>> *scratches head*
>
>     (defun display-completion-list (completions &optional common-substring)
>       [...]
>       (declare (advertised-calling-convention (completions) "24.4"))

Oh, I see.  So should we just remove the optional argument now -- 24.4
was in...  2011?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 17 May 2019 06:29:51 +0200
> Cc: emacs-devel@gnu.org
> 
> >     (defun display-completion-list (completions &optional common-substring)
> >       [...]
> >       (declare (advertised-calling-convention (completions) "24.4"))
> 
> Oh, I see.  So should we just remove the optional argument now -- 24.4
> was in...  2011?

I'd prefer if we made the byte-compiler warnings smarter in this
case.  It shouldn't behave like we are shooting ourselves in the foot
in these cases.

Do we have a means to tell the byte-compiler "don't emit this specific
warning for that function"?  If not, perhaps we should.

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I'd prefer if we made the byte-compiler warnings smarter in this
> case.  It shouldn't behave like we are shooting ourselves in the foot
> in these cases.
>
> Do we have a means to tell the byte-compiler "don't emit this specific
> warning for that function"?  If not, perhaps we should.

Yes, that would be very nice.  Perhaps a general er form?  Like...

(without-byte-compilation-warnings
  ...)

Hm, that's perhaps too general.  What about:

(without-byte-compilation-warning 'callargs
  ...)


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Yes, that would be very nice.  Perhaps a general er form?  Like...
>
> (without-byte-compilation-warnings
>   ...)

Duh.  `with-no-warnings' already exists.  Shall I just slap that around
the call to display-completion-list in

	(let ((standard-output (current-buffer))
	      (completion-setup-hook nil))
	  (display-completion-list completions common-substring))

?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Fri, 17 May 2019 08:12:41 +0200
> 
> > Do we have a means to tell the byte-compiler "don't emit this specific
> > warning for that function"?  If not, perhaps we should.
> 
> Yes, that would be very nice.  Perhaps a general er form?  Like...
> 
> (without-byte-compilation-warnings
>   ...)
> 
> Hm, that's perhaps too general.  What about:
> 
> (without-byte-compilation-warning 'callargs
>   ...)

I thought about something even more focused, like

  (declare 'warning-about-arguments-mismatch 'off)

inside the function's body.

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I thought about something even more focused, like
>
>   (declare 'warning-about-arguments-mismatch 'off)
>
> inside the function's body.

Would that switch off warnings about all argument mismatching in the
function's body?  Because that sounds a bit drastic, too, since you
would want to have warnings about all other function calls with argument
mismatches...

  (declare 'warning-about-arguments-mismatch 'display-completion-list 'off)

perhaps?

But there's other things you may want to disable warnings about that
perhaps won't be that easy to express, so `with-no-warnings' seems more
focused and convenient...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Noam Postavsky]

On Fri, 17 May 2019 at 06:11, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> > I thought about something even more focused, like
> >
> >   (declare 'warning-about-arguments-mismatch 'off)
> >
> > inside the function's body.
>
> Would that switch off warnings about all argument mismatching in the
> function's body?  Because that sounds a bit drastic, too, since you
> would want to have warnings about all other function calls with argument
> mismatches...
>
>   (declare 'warning-about-arguments-mismatch 'display-completion-list 'off)
>
> perhaps?

This makes sense, but it should come for free. There's never any point
in issuing a warning about not using the advertised calling convention
from inside the function itself. Similarly, we shouldn't issue
warnings about calls to obsolete functions from bodies of functions
that are themselves obsolete.

> But there's other things you may want to disable warnings about that
> perhaps won't be that easy to express, so `with-no-warnings' seems more
> focused and convenient...

https://debbugs.gnu.org/4837 "with-fewer-warnings ?" talks about
adding something even more focused than with-no-warnings.

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> This makes sense, but it should come for free. There's never any point
> in issuing a warning about not using the advertised calling convention
> from inside the function itself. Similarly, we shouldn't issue
> warnings about calls to obsolete functions from bodies of functions
> that are themselves obsolete.

Yes, both of those things makes total sense, I think.

>> But there's other things you may want to disable warnings about that
>> perhaps won't be that easy to express, so `with-no-warnings' seems more
>> focused and convenient...
>
> https://debbugs.gnu.org/4837 "with-fewer-warnings ?" talks about
> adding something even more focused than with-no-warnings.

`with-no-warnings' is a pretty blunt instrument; yes.  Stefan and Glenn
(in that thread) suggests the super-specific

  (with-suppressed-warnings
      '((obsolete read-file-name-predicate ...))
    body)

form, which makes sense to me.

Would implementing this stuff be difficult, I wonder?  I've never had
much of a gander at the byte compilation stuff.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

I had a whack at suppressing warnings about obsolete functions being
called from other obsolete functions, and it seems to work for my test
case.

A very similar case can be added for the advertised calling convention
case, I think (where a function calls itself recursively with the
"wrong" convention).

If we want to go in this direction, I can finish it up and add some
documentation...

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index e76baf5ed0..893171f4f5 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -337,12 +337,19 @@ byte-compile-warnings
        (or (symbolp v)
            (null (delq nil (mapcar (lambda (x) (not (symbolp x))) v))))))
 
-(defun byte-compile-warning-enabled-p (warning)
+(defun byte-compile-warning-enabled-p (warning &optional symbol)
   "Return non-nil if WARNING is enabled, according to `byte-compile-warnings'."
-  (or (eq byte-compile-warnings t)
-      (if (eq (car byte-compile-warnings) 'not)
-          (not (memq warning byte-compile-warnings))
-        (memq warning byte-compile-warnings))))
+  ;; Don't issue a warning for SYMBOL being obsolete if called from
+  ;; within an obsolete function.
+  (and (not (and (eq warning 'obsolete)
+                 byte-compile-current-form
+                 (eq byte-compile-current-form symbol)
+                 (get byte-compile-current-form 'byte-obsolete-info)
+                 (not (memq symbol byte-compile-not-obsolete-funcs))))
+       (or (eq byte-compile-warnings t)
+           (if (eq (car byte-compile-warnings) 'not)
+               (not (memq warning byte-compile-warnings))
+             (memq warning byte-compile-warnings)))))
 
 ;;;###autoload
 (defun byte-compile-disable-warning (warning)
@@ -1268,7 +1275,7 @@ byte-compile-warn
 
 (defun byte-compile-warn-obsolete (symbol)
   "Warn that SYMBOL (a variable or function) is obsolete."
-  (when (byte-compile-warning-enabled-p 'obsolete)
+  (when (byte-compile-warning-enabled-p 'obsolete symbol)
     (let* ((funcp (get symbol 'byte-obsolete-info))
            (msg (macroexp--obsolete-warning
                  symbol

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

That should be:

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index e76baf5ed0..9502a139d1 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -337,12 +337,19 @@ byte-compile-warnings
        (or (symbolp v)
            (null (delq nil (mapcar (lambda (x) (not (symbolp x))) v))))))
 
-(defun byte-compile-warning-enabled-p (warning)
+(defun byte-compile-warning-enabled-p (warning &optional symbol)
   "Return non-nil if WARNING is enabled, according to `byte-compile-warnings'."
-  (or (eq byte-compile-warnings t)
-      (if (eq (car byte-compile-warnings) 'not)
-          (not (memq warning byte-compile-warnings))
-        (memq warning byte-compile-warnings))))
+  ;; Don't issue a warning for SYMBOL being obsolete if called from
+  ;; within an obsolete function.
+  (and (not (and symbol
+                 (eq warning 'obsolete)
+                 byte-compile-current-form
+                 (get byte-compile-current-form 'byte-obsolete-info)
+                 (not (memq symbol byte-compile-not-obsolete-funcs))))
+       (or (eq byte-compile-warnings t)
+           (if (eq (car byte-compile-warnings) 'not)
+               (not (memq warning byte-compile-warnings))
+             (memq warning byte-compile-warnings)))))
 
 ;;;###autoload
 (defun byte-compile-disable-warning (warning)
@@ -1268,7 +1275,7 @@ byte-compile-warn
 
 (defun byte-compile-warn-obsolete (symbol)
   "Warn that SYMBOL (a variable or function) is obsolete."
-  (when (byte-compile-warning-enabled-p 'obsolete)
+  (when (byte-compile-warning-enabled-p 'obsolete symbol)
     (let* ((funcp (get symbol 'byte-obsolete-info))
            (msg (macroexp--obsolete-warning
                  symbol

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Hm.  There seems to be some sequencing stuff that's just downright odd.

If I have

(defun minibuffer-completion-contents-1 ()
  (declare (obsolete nil "24.4"))
  (minibuffer-completion-contents-2)
  (buffer-substring (minibuffer-prompt-end) (point)))

(defun minibuffer-completion-contents-2 ()
  (declare (obsolete nil "24.4"))
  (minibuffer-completion-contents-1)
  (buffer-substring (minibuffer-prompt-end) (point)))

then I get

make[1]: Entering directory '/home/larsi/src/emacs/trunk/lisp'
  ELC      minibuffer.elc

In minibuffer-completion-contents-2:
minibuffer.el:743:4:Warning: ‘minibuffer-completion-contents-1’ is an obsolete
    function (as of 24.4).

So I'm able to suppress the first obsolete-to-obsolete call, but not the
second, which...  means that I understand even less about how files are
byte-compiled, and I knew that I knew little.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> (defun minibuffer-completion-contents-1 ()
>   (declare (obsolete nil "24.4"))
>   (minibuffer-completion-contents-2)
>   (buffer-substring (minibuffer-prompt-end) (point)))
>
> (defun minibuffer-completion-contents-2 ()
>   (declare (obsolete nil "24.4"))
>   (minibuffer-completion-contents-1)
>   (buffer-substring (minibuffer-prompt-end) (point)))

Oh, I think I understand now.  While compiling a function, we haven't
yet registered that it's obsolete.

And when -1 is compiled, it doesn't know that -2 is obsolete.  But when
-2 is compiled, it does know that -1 is obsolete, so it all makes sense.

But this means that as of present, if you have an obsolete function in a
file, and you call it from a function defined before that obsolete
function, you won't get the warning, so this seems like a bug...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Oh, I think I understand now.  While compiling a function, we haven't
> yet registered that it's obsolete.

The following would fix that, so it still seems to be pretty easy to
implement the "don't warn about obsolete from obsolete functions".  But
I don't know whether there are other side effects here...

diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index 842d1d48b4..c35aab2e02 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -92,6 +92,9 @@ defun-declarations-alist
                    (list 'quote f) (list 'quote arglist) (list 'quote when))))
    (list 'obsolete
          #'(lambda (f _args new-name when)
+             ;; Record obsolete info immediately so that we know that
+             ;; we're compiling an obsolete function.
+             (put f 'byte-obsolete-info (list new-name nil when))
              (list 'make-obsolete
                    (list 'quote f) (list 'quote new-name) (list 'quote when))))
    (list 'interactive-only


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

I just went ahead and finished up the two "automatic" warning
suppression things that Noam mentioned: With the following patch, a
function that has an advertised calling convention that calls itself
will check the real arglist when issuing warnings, and obsolete
functions that calls other obsolete functions won't issue any warnings.

I've built an Emacs (after rm-ing lisp/*.elc) and things seem to work
fine...

diff --git a/doc/lispref/functions.texi b/doc/lispref/functions.texi
index 97f7fb9f79..5115edddde 100644
--- a/doc/lispref/functions.texi
+++ b/doc/lispref/functions.texi
@@ -2094,6 +2094,11 @@ Obsolete Functions
 @end example
 @end defun
 
+If a function that's marked as obsolete calls another function that's
+marked as obsolete, warnings will not be issued.  Similarly, if a
+function has declared a new calling convention, but calls itself using
+the old argument list, no warning will be output.
+
 @node Inline Functions
 @section Inline Functions
 @cindex inline functions
diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index 842d1d48b4..a7c0bb2eff 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -92,6 +92,11 @@ defun-declarations-alist
                    (list 'quote f) (list 'quote arglist) (list 'quote when))))
    (list 'obsolete
          #'(lambda (f _args new-name when)
+             ;; Record obsolete info immediately so that we know that
+             ;; we're compiling an obsolete function and can avoid
+             ;; giving warnings about calls to obsolete functions from
+             ;; this function.
+             (make-obsolete f new-name when)
              (list 'make-obsolete
                    (list 'quote f) (list 'quote new-name) (list 'quote when))))
    (list 'interactive-only
diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index e76baf5ed0..ae8fc440fb 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -337,12 +337,19 @@ byte-compile-warnings
        (or (symbolp v)
            (null (delq nil (mapcar (lambda (x) (not (symbolp x))) v))))))
 
-(defun byte-compile-warning-enabled-p (warning)
+(defun byte-compile-warning-enabled-p (warning &optional symbol)
   "Return non-nil if WARNING is enabled, according to `byte-compile-warnings'."
-  (or (eq byte-compile-warnings t)
-      (if (eq (car byte-compile-warnings) 'not)
-          (not (memq warning byte-compile-warnings))
-        (memq warning byte-compile-warnings))))
+  ;; Don't issue a warning for SYMBOL being obsolete if called from
+  ;; within an obsolete function.
+  (and (not (and symbol
+                 (eq warning 'obsolete)
+                 byte-compile-current-form
+                 (get byte-compile-current-form 'byte-obsolete-info)
+                 (not (memq symbol byte-compile-not-obsolete-funcs))))
+       (or (eq byte-compile-warnings t)
+           (if (eq (car byte-compile-warnings) 'not)
+               (not (memq warning byte-compile-warnings))
+             (memq warning byte-compile-warnings)))))
 
 ;;;###autoload
 (defun byte-compile-disable-warning (warning)
@@ -1268,7 +1275,7 @@ byte-compile-warn
 
 (defun byte-compile-warn-obsolete (symbol)
   "Warn that SYMBOL (a variable or function) is obsolete."
-  (when (byte-compile-warning-enabled-p 'obsolete)
+  (when (byte-compile-warning-enabled-p 'obsolete symbol)
     (let* ((funcp (get symbol 'byte-obsolete-info))
            (msg (macroexp--obsolete-warning
                  symbol
@@ -1290,7 +1297,7 @@ byte-compile-report-error
 \f
 ;;; sanity-checking arglists
 
-(defun byte-compile-fdefinition (name macro-p)
+(defun byte-compile-fdefinition (name macro-p &optional ignore-advertised)
   ;; If a function has an entry saying (FUNCTION . t).
   ;; that means we know it is defined but we don't know how.
   ;; If a function has an entry saying (FUNCTION . nil),
@@ -1314,7 +1321,8 @@ byte-compile-fdefinition
                                        fn)
                                      advertised-signature-table t)))
             (cond
-             ((listp advertised)
+             ((and (listp advertised)
+                   (not ignore-advertised))
               (if macro-p
                   `(macro lambda ,advertised)
                 `(lambda ,advertised)))
@@ -1403,6 +1411,13 @@ byte-compile-callargs-warn
 		  (byte-compile-fdefinition (car form) t)))
 	 (sig (byte-compile--function-signature def))
 	 (ncall (length (cdr form))))
+    ;; If we're in a recursive function, then ignore the advertised
+    ;; callargs when issuing warnings.
+    (when (and byte-compile-current-form
+               (eq byte-compile-current-form (car form)))
+      (setq def (or (byte-compile-fdefinition (car form) nil t)
+		    (byte-compile-fdefinition (car form) t t))
+            sig (byte-compile--function-signature def)))
     ;; Check many or unevalled from subr-arity.
     (if (and (cdr-safe sig)
 	     (not (numberp (cdr sig))))

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Noam Postavsky]

On Fri, 17 May 2019 at 10:37, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
> I just went ahead and finished up the two "automatic" warning
> suppression things that Noam mentioned: With the following patch, a
> function that has an advertised calling convention that calls itself
> will check the real arglist when issuing warnings, and obsolete
> functions that calls other obsolete functions won't issue any warnings.
>
> I've built an Emacs (after rm-ing lisp/*.elc) and things seem to work

> @@ -92,6 +92,11 @@ defun-declarations-alist
>                     (list 'quote f) (list 'quote arglist) (list 'quote when))))
>     (list 'obsolete
>           #'(lambda (f _args new-name when)
> +             ;; Record obsolete info immediately so that we know that
> +             ;; we're compiling an obsolete function and can avoid
> +             ;; giving warnings about calls to obsolete functions from
> +             ;; this function.
> +             (make-obsolete f new-name when)
>               (list 'make-obsolete
>                     (list 'quote f) (list 'quote new-name) (list 'quote when))))

I think this could have the unwanted side-effect of "polluting" the
Emacs session which runs the compiler with the obsolete function info
from the files it's compiling. If we have with-suppressed-warnings
then this auto suppression could be implemented in a more obvious way
by wrapping the output code in (with-suppressed-warnings ...). And
perhaps as Stefan suggests, we'll find the automation isn't really
worth the trouble anyway.

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> I think this could have the unwanted side-effect of "polluting" the
> Emacs session which runs the compiler with the obsolete function info
> from the files it's compiling.

That's true, but does it matter?

> If we have with-suppressed-warnings then this auto suppression could
> be implemented in a more obvious way by wrapping the output code in
> (with-suppressed-warnings ...). 

Hm...  You mean that if we have an obsolete declaration, then we alter
the defun to be (in effect)

(defun foo ()
  (with-suppressed-warnings (obsolete foo)
    ...))

That seems like a rather drastic way of doing it, I think?

> And perhaps as Stefan suggests, we'll find the automation isn't really
> worth the trouble anyway.

Well, it didn't seem to be much trouble as the implementation already
works.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Noam Postavsky]

On Tue, 28 May 2019 at 05:49, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
> Noam Postavsky <npostavs@gmail.com> writes:
>
> > I think this could have the unwanted side-effect of "polluting" the
> > Emacs session which runs the compiler with the obsolete function info
> > from the files it's compiling.
>
> That's true, but does it matter?

Not sure, but after spending time on preventing defsetf from defining
things in the compiler's Emacs session[1], I would feel bad about
letting this one through ;)

[1]: https://debbugs.gnu.org/27016

> (defun foo ()
>   (with-suppressed-warnings (obsolete foo)
>     ...))
>
> That seems like a rather drastic way of doing it, I think?

Why do you say that?

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

>> (defun foo ()
>>   (with-suppressed-warnings (obsolete foo)
>>     ...))
>>
>> That seems like a rather drastic way of doing it, I think?
>
> Why do you say that?

It introduces another progn level...  Perhaps that doesn't matter?  Or
is removed by the compiler?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Noam Postavsky]

On Tue, 28 May 2019 at 07:26, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> >> (defun foo ()
> >>   (with-suppressed-warnings (obsolete foo)
> >>     ...))

> It introduces another progn level...  Perhaps that doesn't matter?  Or
> is removed by the compiler?

Yep, try: (disassemble (lambda (x) (progn (progn (progn (progn (progn x)))))))

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> On Tue, 28 May 2019 at 07:26, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
>> >> (defun foo ()
>> >>   (with-suppressed-warnings (obsolete foo)
>> >>     ...))
>
>> It introduces another progn level...  Perhaps that doesn't matter?  Or
>> is removed by the compiler?
>
> Yep, try: (disassemble (lambda (x) (progn (progn (progn (progn (progn x)))))))

Cool.  Would it be difficult to have the byte compiler do the
add-the-with-suppressed-warnings thing?

And Eli hasn't OK'd the with-suppressed-warnings yet.  :-)  Did that
patch look OK?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

>> Yep, try: (disassemble (lambda (x) (progn (progn (progn (progn (progn x)))))))

Note that this is never the full story: the presence of extra code
that's removed during compilation can still result in different code
because it can hide optimization opportunities (tho this always depends
on relative ordering of the various optimizations involved).


        Stefan

Re: Towards a cleaner build

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Stefan Monnier <monnier@iro.umontreal.ca>,  Emacs developers <emacs-devel@gnu.org>
> Date: Tue, 28 May 2019 13:39:46 +0200
> 
> And Eli hasn't OK'd the with-suppressed-warnings yet.  :-)  Did that
> patch look OK?

Not sure what this alludes to.  I may have missed some question?

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: Eli Zaretskii <eliz@gnu.org>, Stefan Monnier
>> <monnier@iro.umontreal.ca>, Emacs developers <emacs-devel@gnu.org>
>> Date: Tue, 28 May 2019 13:39:46 +0200
>> 
>> And Eli hasn't OK'd the with-suppressed-warnings yet.  :-)  Did that
>> patch look OK?
>
> Not sure what this alludes to.  I may have missed some question?

It's this patch.  It adds the syntax

  (with-suppressed-warnings (obsolete some-function)
    (some-function la la la))

which is very similar to with-no-warnings, only it specifies precisely
which warning on what function we're suppressing.

diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index 842d1d48b4..9a9d93367b 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -494,6 +494,18 @@ with-no-warnings
   ;; The implementation for the interpreter is basically trivial.
   (car (last body)))
 
+(defmacro with-suppressed-warnings (warnings &rest body)
+  "Like `progn', but prevents compiler warnings in the body."
+  (declare (indent 1))
+  ;; The implementation for the interpreter is basically trivial.
+  `(with-suppressed-warnings-1 ',warnings (progn ,@body)))
+
+(defun with-suppressed-warnings-1 (_ &rest body)
+  "Like `progn', but prevents compiler warnings in the body."
+  (declare (indent 1))
+  ;; The implementation for the interpreter is basically trivial.
+  (car (last body)))
+
 \f
 (defun byte-run--unescaped-character-literals-warning ()
   "Return a warning about unescaped character literals.
diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index e76baf5ed0..1285ef9037 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -331,18 +331,26 @@ byte-compile-warnings
                       ,@(mapcar (lambda (x) `(const ,x))
                                 byte-compile-warning-types))))
 
+(defvar byte-compile-suppressed-warnings nil)
+
 ;;;###autoload
 (put 'byte-compile-warnings 'safe-local-variable
      (lambda (v)
        (or (symbolp v)
            (null (delq nil (mapcar (lambda (x) (not (symbolp x))) v))))))
 
-(defun byte-compile-warning-enabled-p (warning)
+(defun byte-compile-warning-enabled-p (warning &optional symbol)
   "Return non-nil if WARNING is enabled, according to `byte-compile-warnings'."
-  (or (eq byte-compile-warnings t)
-      (if (eq (car byte-compile-warnings) 'not)
-          (not (memq warning byte-compile-warnings))
-        (memq warning byte-compile-warnings))))
+  (let ((suppress nil))
+    (dolist (elem byte-compile-suppressed-warnings)
+      (when (and (eq (car elem) warning)
+                 (memq symbol (cdr elem)))
+        (setq suppress t)))
+    (and (not suppress)
+         (or (eq byte-compile-warnings t)
+             (if (eq (car byte-compile-warnings) 'not)
+                 (not (memq warning byte-compile-warnings))
+               (memq warning byte-compile-warnings))))))
 
 ;;;###autoload
 (defun byte-compile-disable-warning (warning)
@@ -2518,6 +2526,15 @@ byte-compile-file-form-with-no-warnings
     (mapc 'byte-compile-file-form (cdr form))
     nil))
 
+(put 'with-suppressed-warnings-1 'byte-hunk-handler
+     'byte-compile-file-form-with-suppressed-warnings)
+(defun byte-compile-file-form-with-suppressed-warnings (form)
+  ;; cf byte-compile-file-form-progn.
+  (let ((byte-compile-suppressed-warnings
+         (append (cadadr form) byte-compile-suppressed-warnings)))
+    (mapc 'byte-compile-file-form (cddr form))
+    nil))
+
 ;; Automatically evaluate define-obsolete-function-alias etc at top-level.
 (put 'make-obsolete 'byte-hunk-handler 'byte-compile-file-form-make-obsolete)
 (defun byte-compile-file-form-make-obsolete (form)
@@ -3150,7 +3167,7 @@ byte-compile-form
         (when (and (byte-compile-warning-enabled-p 'suspicious)
                    (macroexp--const-symbol-p fn))
           (byte-compile-warn "`%s' called as a function" fn))
-	(when (and (byte-compile-warning-enabled-p 'interactive-only)
+	(when (and (byte-compile-warning-enabled-p 'interactive-only fn)
 		   interactive-only)
 	  (byte-compile-warn "`%s' is for interactive use only%s"
 			     fn
@@ -4765,6 +4782,14 @@ byte-compile-no-warnings
   (let (byte-compile-warnings)
     (byte-compile-form (cons 'progn (cdr form)))))
 
+(byte-defop-compiler-1 with-suppressed-warnings-1
+                       byte-compile-suppressed-warnings)
+(defun byte-compile-suppressed-warnings (form)
+  (let ((byte-compile-suppressed-warnings
+         (append (cadadr form) byte-compile-suppressed-warnings)))
+    (mapc 'byte-compile-file-form (cddr form))
+    nil))
+
 ;; Warn about misuses of make-variable-buffer-local.
 (byte-defop-compiler-1 make-variable-buffer-local
                        byte-compile-make-variable-buffer-local)


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: npostavs@gmail.com,  monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Tue, 28 May 2019 17:11:28 +0200
> 
> >> And Eli hasn't OK'd the with-suppressed-warnings yet.  :-)  Did that
> >> patch look OK?
> >
> > Not sure what this alludes to.  I may have missed some question?
> 
> It's this patch.  It adds the syntax
> 
>   (with-suppressed-warnings (obsolete some-function)
>     (some-function la la la))
> 
> which is very similar to with-no-warnings, only it specifies precisely
> which warning on what function we're suppressing.

If this is for master, I'm not sure you need my explicit approval.
Or did someone object vociferously?

But if you do push this, please call this macro out in NEWS, and
perhaps also document it in the ELisp manual.

Thanks.

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> If this is for master, I'm not sure you need my explicit approval.
> Or did someone object vociferously?

Nope, everybody who's weighed in seemed to think it made sense, but
since this a language-level new thing, I didn't want to put too much
work on it before you'd said whether you object or not...

> But if you do push this, please call this macro out in NEWS, and
> perhaps also document it in the ELisp manual.

Yes, indeed -- the patch needs a lot of work before it's production
ready, but since you seem kinda not-negative :-) towards it, I think
I'll create a branch and finish the implementation.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Noam Postavsky]

On Tue, 28 May 2019 at 11:11, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> +(defmacro with-suppressed-warnings (warnings &rest body)
> +  "Like `progn', but prevents compiler warnings in the body."

Not quite like progn, since WARNINGS (is that WARNINGS or WARNING, by
the way?) is not evaluated.

> +  (declare (indent 1))

A debug spec would be a good thing to have.

> +  ;; The implementation for the interpreter is basically trivial.
> +  `(with-suppressed-warnings-1 ',warnings (progn ,@body)))

A comment here on why we need both a function and macro would be
useful (or is it possible to have this work with just a macro?)

> +(defun with-suppressed-warnings-1 (_ &rest body)
> +  "Like `progn', but prevents compiler warnings in the body."
> +  (declare (indent 1))

This should be called internal--with-suppressed-warnings-1, and the
(declare (indent 1)) thing left out, since nobody should be calling
this function directly anyway, right?

Re: Towards a cleaner build

[Stefan Monnier]

>> +  ;; The implementation for the interpreter is basically trivial.
>> +  `(with-suppressed-warnings-1 ',warnings (progn ,@body)))
> A comment here on why we need both a function and macro would be
> useful (or is it possible to have this work with just a macro?)

Indeed, for the interpreter the implementation should be like that of
with-no-warnings, just dropping the `warnings` argument altogether.


        Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> On Tue, 28 May 2019 at 11:11, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
>> +(defmacro with-suppressed-warnings (warnings &rest body)
>> +  "Like `progn', but prevents compiler warnings in the body."
>
> Not quite like progn, since WARNINGS (is that WARNINGS or WARNING, by
> the way?) is not evaluated.
>
>> +  (declare (indent 1))
>
> A debug spec would be a good thing to have.

Yup.

>> +  ;; The implementation for the interpreter is basically trivial.
>> +  `(with-suppressed-warnings-1 ',warnings (progn ,@body)))
>
> A comment here on why we need both a function and macro would be
> useful (or is it possible to have this work with just a macro?)

Let's see...  Oh, right.  `with-no-warnings' is a function, while I
thought it would make sense for `with-suppressed-warnings' to be a macro
since the WARNINGS are supposed to be unevaluated.

But as a macro that expands to (basically) `progn', then the byte
compiler just sees the `progn' and is unable to do anything with the
form.  So I needed to make the macro expand to something the byte
compiler can pick up, which is `with-suppressed-warnings-1'.

I'm completely open to me not understanding how the byte compiler
works...  because I don't, really.  :-)  But I tried using just a macro
first, and I couldn't get the byte compiler to understand it.

>> +(defun with-suppressed-warnings-1 (_ &rest body)
>> +  "Like `progn', but prevents compiler warnings in the body."
>> +  (declare (indent 1))
>
> This should be called internal--with-suppressed-warnings-1, and the
> (declare (indent 1)) thing left out, since nobody should be calling
> this function directly anyway, right?

Yup.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

> Let's see...  Oh, right.  `with-no-warnings' is a function, while I
> thought it would make sense for `with-suppressed-warnings' to be a macro
> since the WARNINGS are supposed to be unevaluated.

Yes, a macro makes sense.

> But as a macro that expands to (basically) `progn', then the byte
> compiler just sees the `progn' and is unable to do anything with the
> form.

You can use byte-compile-macro-environment to override the default
definition of the macro during compilation.


        Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> But as a macro that expands to (basically) `progn', then the byte
>> compiler just sees the `progn' and is unable to do anything with the
>> form.
>
> You can use byte-compile-macro-environment to override the default
> definition of the macro during compilation.

Oh, cool.  I'll explore that and get rid of the intermediary function...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> But as a macro that expands to (basically) `progn', then the byte
>> compiler just sees the `progn' and is unable to do anything with the
>> form.
>
> You can use byte-compile-macro-environment to override the default
> definition of the macro during compilation.

I've been poking at the machinery here, and I'm not quite sure I
understand.  If I put a different definition of with-suppressed-warnings
into byte-compile-initial-macro-environment, then that will indeed
override the normal definition.

But that variable is consulted before compilation is done, and it can't
do any bindings that affect the compilation itself, I think?  I mean, it
can alter the global state, but not do any let bindings.

Or did you mean that that should return an intermediary function and
then do the byte-hunk-handler thing as in my code sketch?

*implements*

Oh, wow, that worked.  :-)  It always helps to ask, apparently.

But if you meant something else, please correct my new
misunderstanding.  I've created a new branch for this, and I'll push
when I've cleaned things up slightly.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

I'm working my way through the different byte compilation warnings, and
suppressing is usually trivial.  But then there's this:

(defun cconv--analyze-use (vardata form varkind)
  "Analyze the use of a variable.

[...]

     (unless (or ;; Uninterned symbols typically come from macro-expansion, so
              ;; it is often non-trivial for the programmer to avoid such
              ;; unused vars.
              (not (intern-soft var))
              (eq ?_ (aref (symbol-name var) 0))
	      ;; As a special exception, ignore "ignore".
	      (eq var 'ignored))
       (byte-compile-warn "Unused lexical %s `%S'"
                          varkind var)))

It does not use `byte-compile-warning-enabled-p' at all -- is that a
bug?  It should be able to suppress those via `byte-compile-warnings',
right?

But I can't really see any practical way of using
`with-suppressed-warnings' here...  Unless...

(with-suppressed-warnings ((unused foo))
  (defun bar (foo)
    nil))

?

Looks kinda janky.  Perhaps a `declare' would be more sensible.  Or not
being able to use `with-suppressed-warnings' here.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Noam Postavsky]

On Sun, 9 Jun 2019 at 10:53, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> (with-suppressed-warnings ((unused foo))
>   (defun bar (foo)
>     nil))
>
> ?
>
> Looks kinda janky.  Perhaps a `declare' would be more sensible.  Or not
> being able to use `with-suppressed-warnings' here.

The warning can be suppressed with (ignore foo), so I guess

(defun bar (foo)
  (with-suppressed-warnings ((unused foo))
    nil))

Could expand to

(defun bar (foo)
  (progn
    (ignore foo)
    nil))

Although that does mean you have make sure to only use that within the
scope of foo.

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> The warning can be suppressed with (ignore foo), so I guess
>
> (defun bar (foo)
>   (with-suppressed-warnings ((unused foo))
>     nil))
>
> Could expand to
>
> (defun bar (foo)
>   (progn
>     (ignore foo)
>     nil))
>
> Although that does mean you have make sure to only use that within the
> scope of foo.

Yeah...  But I think perhaps pointing the users to `ignore' here is the
right thing to do.

By the way, this reminds me: Is there any reason why
(declare (ignore foo)) isn't supported?  We've embraced `declare' for
other things, so I think it makes sense to do that for `ignore', too.
(And perhaps then later mark simple `ignore' as obsolete.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

>> The warning can be suppressed with (ignore foo), so I guess
>>
>> (defun bar (foo)
>>   (with-suppressed-warnings ((unused foo))
>>     nil))
>>
>> Could expand to
>>
>> (defun bar (foo)
>>   (progn
>>     (ignore foo)
>>     nil))
>>
>> Although that does mean you have make sure to only use that within the
>> scope of foo.

Usually a better option is to use `_foo` instead of `foo`.

> By the way, this reminds me: Is there any reason why
> (declare (ignore foo)) isn't supported?

Sounds verbose.  Furthermore, the `declare` we have in Elisp is
a declaration that relates to the function/macro being defined rather
than to some chunk of code (it has to come together with the docstring
and the interactive spec).  It's related to yet different from that of
Common Lisp.

> We've embraced `declare' for other things, so I think it makes sense
> to do that for `ignore', too.  (And perhaps then later mark simple
> `ignore' as obsolete.)

`ignore` is a normal function used for various different things.
The fact that it can be used to silence this warning is just a happy
coincidence.

    grep "#'ignore" **/*.el | wc
    
gives me more than a hundred reasons not to mark it obsolete ;-)


        Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Sounds verbose.  Furthermore, the `declare` we have in Elisp is
> a declaration that relates to the function/macro being defined rather
> than to some chunk of code (it has to come together with the docstring
> and the interactive spec).  It's related to yet different from that of
> Common Lisp.

Well, the "unused" bit is something that comes up when defining
functions, so these things seem tied together...

>> We've embraced `declare' for other things, so I think it makes sense
>> to do that for `ignore', too.  (And perhaps then later mark simple
>> `ignore' as obsolete.)
>
> `ignore` is a normal function used for various different things.
> The fact that it can be used to silence this warning is just a happy
> coincidence.

Well, somebody surely has implemented it.  :-)  Somewhere.  I looked for
the code and couldn't find it...

>     grep "#'ignore" **/*.el | wc
>
> gives me more than a hundred reasons not to mark it obsolete ;-)

I meant the special construct; not the function...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

OK; I think the feature is now er feature complete -- it works, and it's
documented and there's even tests.

So if somebody can take a look at the scratch/with-suppressed-warnings
branch and see whether it works for them, and possibly critique the
code, that'd be nice.

There's some discussion about whether to perhaps change the warning
types that we allow suppressing, but the current implementation is a
pure subset of byte-compile-warning-types, except a couple that didn't
make sense as a special form (i.e., the warnings about the entire .el
file as a unit).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 09 Jun 2019 22:09:38 +0200
> 
> OK; I think the feature is now er feature complete -- it works, and it's
> documented and there's even tests.
> 
> So if somebody can take a look at the scratch/with-suppressed-warnings
> branch and see whether it works for them, and possibly critique the
> code, that'd be nice.

I took a cursory look, and I have no significant comments.

Thanks.

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I took a cursory look, and I have no significant comments.

OK; I've now squashed, merged and pushed, and will be deleting the
branch shortly.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

OK, in the toplevel lisp directory there's now only four warnings.  

In toplevel form:
isearchb.el:80:1:Warning: Package iswitchb is obsolete!

But as far as I can tell, the entire point of the isearchb package is to
interact with isiwtchb, so should this also be obsolete?

----

This one I'm not sure what the status is with: It's an obsolete
variable, but it seems unsure how it should be replaced here, if I
remember the discussion correctly:

In pcomplete/tar:
pcmpl-gnu.el:162:47:Warning: `pcomplete-suffix-list' is an obsolete variable
    (as of 24.1).

I'm not sure whether suppressing this warning is the right thing,
either...

----

And the final two Eli was pondering what to do with, so I'll just leave
them as warnings for further pondering.

In gui-get-selection:
select.el:316:23:Warning: `string-to-multibyte' is an obsolete function (as of
    26.1); use `decode-coding-string'.

In xselect--encode-string:
select.el:482:24:Warning: `string-make-unibyte' is an obsolete function (as of
    26.1); use `encode-coding-string'.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

> OK, in the toplevel lisp directory there's now only four warnings.  
>
> In toplevel form:
> isearchb.el:80:1:Warning: Package iswitchb is obsolete!
>
> But as far as I can tell, the entire point of the isearchb package is to
> interact with isiwtchb, so should this also be obsolete?

It is currently built on top of iswitchb and offers a way to "fallback"
to iswitchb, but it could/should be rewritten not to rely on iswitchb.

> This one I'm not sure what the status is with: It's an obsolete
> variable, but it seems unsure how it should be replaced here,

Right, it should be replaced by obsoleting the `pcomplete` command (not
the package).

> In pcomplete/tar:
> pcmpl-gnu.el:162:47:Warning: `pcomplete-suffix-list' is an obsolete variable
>     (as of 24.1).
>
> I'm not sure whether suppressing this warning is the right thing,
> either...

Agreed.

> And the final two Eli was pondering what to do with, so I'll just leave
> them as warnings for further pondering.
>
> In gui-get-selection:
> select.el:316:23:Warning: `string-to-multibyte' is an obsolete function (as of
>     26.1); use `decode-coding-string'.

I think, just as for string-to-unibyte, we likely should un-obsolete string-to-multibyte.
Arguably, here we could just return the unibyte string without passing
it through string-to-multibyte, tho.

> In xselect--encode-string:
> select.el:482:24:Warning: `string-make-unibyte' is an obsolete function (as of
>     26.1); use `encode-coding-string'.

This one should be string-to-unibyte.


        Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> In toplevel form:
>> isearchb.el:80:1:Warning: Package iswitchb is obsolete!
>>
>> But as far as I can tell, the entire point of the isearchb package is to
>> interact with isiwtchb, so should this also be obsolete?
>
> It is currently built on top of iswitchb and offers a way to "fallback"
> to iswitchb, but it could/should be rewritten not to rely on iswitchb.

I see...  rewrite to use ido.el or something instead?  I haven't used
isearchb, so I'm a bit vague about what it does...

> I think, just as for string-to-unibyte, we likely should un-obsolete
> string-to-multibyte.

Yes, I think so too -- they are sometimes what one needs to do...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

> I see...  rewrite to use ido.el or something instead?

I don't see why it can't work with the standard completion system.

> I haven't used isearchb, so I'm a bit vague about what it does...

It's kind of like iswitchb except it performs the switch(es) as you type
the name and it offers the possibility to be used "with a modifier
instead of a key-binding".


        Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> I think, just as for string-to-unibyte, we likely should un-obsolete
>> string-to-multibyte.
>
> Yes, I think so too -- they are sometimes what one needs to do...

This has been mooted a few times in the past, too, and nobody has
protested.  So I'll go ahead and remove the obsoletions of those two
functions (and note it in NEWS).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>>> I think, just as for string-to-unibyte, we likely should un-obsolete
>>> string-to-multibyte.
>>
>> Yes, I think so too -- they are sometimes what one needs to do...
>
> This has been mooted a few times in the past, too, and nobody has
> protested.  So I'll go ahead and remove the obsoletions of those two
> functions (and note it in NEWS).

Oh, I missed that string-to-unibyte had already been unobsoleted, but
string-to-multibyte hadn't.  So perhaps there were objections to that
part that I missed?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: rmailmm

[Lars Ingebrigtsen]

In rmail-mime-insert-bulk:
mail/rmailmm.el:839:21:Warning: `string-as-unibyte' is an obsolete function
    (as of 26.1); use `encode-coding-string'.

So, it's this code:

(defun rmail-mime-insert-bulk (entity)
  "Presentation handler for an attachment MIME entity."

[...]

      (if (stringp (aref body 0))
	  (setq data (aref body 0))
	(setq data (string-as-unibyte (buffer-string)))
	(aset body 0 data)
	(rmail-mime-set-bulk-data entity)
	(delete-region (point-min) (point-max)))

I've stared at the code some, but I'm not totally sure what it's doing
here.  But...  wouldn't string-to-unibyte be the right thing here
anyway?

I'm not an rmail user, so I can't test this.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: rmailmm

[Stefan Monnier]

> In rmail-mime-insert-bulk:
> mail/rmailmm.el:839:21:Warning: `string-as-unibyte' is an obsolete function
>     (as of 26.1); use `encode-coding-string'.
[...]
> I've stared at the code some, but I'm not totally sure what it's doing
> here.

Yeah, I looked at the history and that didn't enlighten me either (it
was introduced in 2e9075d3968a2542d47b7e64a1e457711568373d, i.e. soon
after the release of Emacs-23.1 where the internal encoding
changed to utf-8 and Rmail changed from babyl to mbox format).

> But...  wouldn't string-to-unibyte be the right thing here anyway?

I'd tend to agree.  string-as-unibyte ends up basically encoding with
`utf-8-emacs-unix` coding-system which seems like an odd choice here.

> I'm not an rmail user, so I can't test this.

Neither am I.


        Stefan

Re: Towards a cleaner build: rmailmm

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 13 Jun 2019 18:15:31 -0400
> 
> > But...  wouldn't string-to-unibyte be the right thing here anyway?
> 
> I'd tend to agree.  string-as-unibyte ends up basically encoding with
> `utf-8-emacs-unix` coding-system which seems like an odd choice here.

I think we should simply remove that call and use the result of
buffer-string directly.  The buffer whose text we are receiving from
buffer-string is a unibyte buffer, so the string will also be unibyte.
I just tried this on my recent mail and didn't see any problems.

> > I'm not an rmail user, so I can't test this.
> 
> Neither am I.

I am.

Re: Towards a cleaner build: rmailmm

[Stefan Monnier]

> I think we should simply remove that call and use the result of
> buffer-string directly.

Even better,


        Stefan

Re: Towards a cleaner build: rmailmm

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I think we should simply remove that call and use the result of
> buffer-string directly.  The buffer whose text we are receiving from
> buffer-string is a unibyte buffer, so the string will also be unibyte.
> I just tried this on my recent mail and didn't see any problems.

OK; I'll remove the string-as-unibyte call (but I'll add an assert about
the unibyteness in case there are some call sequences that leave the
buffer somehow multibyte).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: eieio

[Lars Ingebrigtsen]

In toplevel form:
emacs-lisp/eieio.el:851:13:Warning: `object-print' is an obsolete function (as
    of 26.1); use `cl-print-object' instead.

It's this code.

(cl-defmethod cl-print-object ((object eieio-default-superclass) stream)
  "Default printer for EIEIO objects."
  ;; Fallback to the old `object-print'.
  (princ (object-print object) stream))

But:

(cl-defgeneric object-print (this &rest _strings)
  "Pretty printer for object THIS.

[...]

  (declare (obsolete cl-print-object "26.1"))
  (format "%S" this))

Can't cl-print-object for eieio-default-superclass just call format
directly?  Am I missing something subtle here?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: eieio

[Stefan Monnier]

> (cl-defmethod cl-print-object ((object eieio-default-superclass) stream)
>   "Default printer for EIEIO objects."
>   ;; Fallback to the old `object-print'.
>   (princ (object-print object) stream))

This is backward compatibility for those packages that define an
`object-print` method on their own eieio classes (instead of a method
on cl-print-object).


        Stefan

Re: Towards a cleaner build: eieio

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> (cl-defmethod cl-print-object ((object eieio-default-superclass) stream)
>>   "Default printer for EIEIO objects."
>>   ;; Fallback to the old `object-print'.
>>   (princ (object-print object) stream))
>
> This is backward compatibility for those packages that define an
> `object-print` method on their own eieio classes (instead of a method
> on cl-print-object).

Yes, but what I wandered was about the call to `object-print'.  It seems
like the definition of object-print here is kinda trivial:

(cl-defmethod object-print ((this eieio-default-superclass) &rest strings)

[...]

  (eieio-object-name this (apply #'concat strings)))

So couldn't cl-print-object just say

  (eieio-object-name object)

instead of

  (object-print object)

in the body and avoid the compilation warning?  Or am I misreading the
code?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: eieio

[Stefan Monnier]

>>> (cl-defmethod cl-print-object ((object eieio-default-superclass) stream)
>>>   "Default printer for EIEIO objects."
>>>   ;; Fallback to the old `object-print'.
>>>   (princ (object-print object) stream))
>>
>> This is backward compatibility for those packages that define an
>> `object-print` method on their own eieio classes (instead of a method
>> on cl-print-object).
>
> Yes, but what I wandered was about the call to `object-print'.  It seems
> like the definition of object-print here is kinda trivial:
>
> (cl-defmethod object-print ((this eieio-default-superclass) &rest strings)
>
> [...]
>
>   (eieio-object-name this (apply #'concat strings)))

This is just the default definition.  Packages can add their own methods
to it.

> So couldn't cl-print-object just say
>
>   (eieio-object-name object)
>
> instead of
>
>   (object-print object)

That wouldn't obey the methods added to object-print.


        Stefan

Re: Towards a cleaner build: eieio

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> (cl-defmethod object-print ((this eieio-default-superclass) &rest strings)
>>
>> [...]
>>
>>   (eieio-object-name this (apply #'concat strings)))
>
> This is just the default definition.  Packages can add their own methods
> to it.

Oh, oh, I see.  I wasn't aware that there were still specialisations to
object-print all over the place, like

(cl-defmethod object-print ((obj semanticdb-abstract-table) &rest strings)
  ...)

What's the path forward here?  I don't think there are any calls to
object-print left in the code base (except from that cl-print-object)...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: eieio

[Stefan Monnier]

> What's the path forward here?

It's been obsolete since 26.1, so I think we should just wait.


        Stefan

Re: Towards a cleaner build: eieio

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> What's the path forward here?
>
> It's been obsolete since 26.1, so I think we should just wait.

Sorry, I meant -- are all these object-print methods supposed to be
expressed as cl-print-object methods instead?  And if so, can they be
rewritten that way now without breaking anything?  Or would that break
out-of-tree code that calls object-print?

eieio.el has general fallback code for object-print, but does not
consult the output of cl-print-object for the objects, so I guess that
might break something?  But...  could we do that, without going all
recursive?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: bindat

[Lars Ingebrigtsen]

We're on the home stretch now...

In bindat--unpack-item:
emacs-lisp/bindat.el:267:10:Warning: `string-make-unibyte' is an obsolete
    function (as of 26.1); use `encode-coding-string'.
emacs-lisp/bindat.el:264:62:Warning: `string-make-unibyte' is an obsolete
    function (as of 26.1); use `encode-coding-string'.

It's this code:

   ((eq type 'str)
    (let ((s (substring bindat-raw bindat-idx (+ bindat-idx len))))
      (setq bindat-idx (+ bindat-idx len))
      (if (stringp s) s
	(string-make-unibyte (concat s)))))
   ((eq type 'strz)
    (let ((i 0) s)
      (while (and (< i len) (/= (aref bindat-raw (+ bindat-idx i)) 0))
	(setq i (1+ i)))
      (setq s (substring bindat-raw bindat-idx (+ bindat-idx i)))
      (setq bindat-idx (+ bindat-idx len))
      (if (stringp s) s
	(string-make-unibyte (concat s)))))

s is either a string or a vector, and if it's a vector, we transform it
to a unibyte string by running it through concat and
string-make-unibyte.

But as far as I can tell from poking at the code, the vector will always
be a vector of byte values; i.e., in the 0-255 region.  concat, indeed,
returns a multibyte string:

(multibyte-string-p (concat [250 255]))
=> t

So if we just avoid the concat and make a unibyte string from the vector
in, well, just about any other way, then the string-make-unibyte
should be unnecessary, right?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: bindat

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sat, 15 Jun 2019 16:04:08 +0200
> 
>    ((eq type 'str)
>     (let ((s (substring bindat-raw bindat-idx (+ bindat-idx len))))
>       (setq bindat-idx (+ bindat-idx len))
>       (if (stringp s) s
> 	(string-make-unibyte (concat s)))))
>    ((eq type 'strz)
>     (let ((i 0) s)
>       (while (and (< i len) (/= (aref bindat-raw (+ bindat-idx i)) 0))
> 	(setq i (1+ i)))
>       (setq s (substring bindat-raw bindat-idx (+ bindat-idx i)))
>       (setq bindat-idx (+ bindat-idx len))
>       (if (stringp s) s
> 	(string-make-unibyte (concat s)))))
> 
> s is either a string or a vector, and if it's a vector, we transform it
> to a unibyte string by running it through concat and
> string-make-unibyte.
> 
> But as far as I can tell from poking at the code, the vector will always
> be a vector of byte values; i.e., in the 0-255 region.  concat, indeed,
> returns a multibyte string:
> 
> (multibyte-string-p (concat [250 255]))
> => t
> 
> So if we just avoid the concat and make a unibyte string from the vector
> in, well, just about any other way, then the string-make-unibyte
> should be unnecessary, right?

Yes.  You should create the string by calling unibyte-string.

Re: Towards a cleaner build: bindat

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Yes.  You should create the string by calling unibyte-string.

I've now done this change, and I've also added a bindat-tests file to
check the results, and the change does not seem to introduce any
regressions, so I'm pushing the change.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: bindat

[Lars Ingebrigtsen]

This one's funny:

In toplevel form:
mail/feedmail.el:2151:1:Warning: Unused lexical argument `helper'


(defun feedmail-queue-send-edit-prompt-inner (default prompt reprompt helper
					      standard-alist user-alist)

[...]

	  (if (= user-sez help-char)
              ;; FIXME: This seems to want to refer to the `helper' argument,
              ;; but it's quoted so the `helper' arg ends up unused!
	      (setq answer '(^ . helper))

[...]

      (cdr answer))))


The only call to this function is

  (feedmail-queue-send-edit-prompt-inner
   feedmail-ask-before-queue-default
   feedmail-ask-before-queue-prompt
   feedmail-ask-before-queue-reprompt
   'feedmail-message-action-help
   feedmail-prompt-before-queue-standard-alist
   feedmail-prompt-before-queue-user-alist
   ))

so `helper' is always `feedmail-message-action-help', but as the comment
says, it's never used, but instead the symbol `helper' is returned from
the function, and it's then funcalled:

  (if (not feedmail-enable-queue) (feedmail-send-it-immediately-wrapper)
    ;; else, queuing is enabled, should we ask about it or just do it?
    (if feedmail-ask-before-queue
	(funcall (feedmail-queue-send-edit-prompt))
      (feedmail-dump-message-to-queue feedmail-queue-directory 'after-queue)))

So this seems like a genuine bug.

Are there any feedmail users here?  Is so, could you
(setq feedmail-enable-queue t) and see whether you get an error when
sending mail?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: bindat

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sat, 15 Jun 2019 16:50:03 +0200
> 
>   (feedmail-queue-send-edit-prompt-inner
>    feedmail-ask-before-queue-default
>    feedmail-ask-before-queue-prompt
>    feedmail-ask-before-queue-reprompt
>    'feedmail-message-action-help
>    feedmail-prompt-before-queue-standard-alist
>    feedmail-prompt-before-queue-user-alist
>    ))
> 
> so `helper' is always `feedmail-message-action-help', but as the comment
> says, it's never used, but instead the symbol `helper' is returned from
> the function, and it's then funcalled:
> 
>   (if (not feedmail-enable-queue) (feedmail-send-it-immediately-wrapper)
>     ;; else, queuing is enabled, should we ask about it or just do it?
>     (if feedmail-ask-before-queue
> 	(funcall (feedmail-queue-send-edit-prompt))
>       (feedmail-dump-message-to-queue feedmail-queue-directory 'after-queue)))
> 
> So this seems like a genuine bug.

Sounds like this bug was caused by converting feedmail to
lexical-binding?

Re: Towards a cleaner build: feedmail

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Sounds like this bug was caused by converting feedmail to
> lexical-binding?

Hm...  I don't think so: It's funcalling the wrong symbol, in essence
doing

(funcall (let ((helper 'message)) 'helper))

I think the fix is pretty trivial:

diff --git a/lisp/mail/feedmail.el b/lisp/mail/feedmail.el
index 943bdd8851..6f50c3e4ed 100644
--- a/lisp/mail/feedmail.el
+++ b/lisp/mail/feedmail.el
@@ -2170,7 +2170,7 @@ feedmail-queue-send-edit-prompt-inner
 	  (if (= user-sez help-char)
               ;; FIXME: This seems to want to refer to the `helper' argument,
               ;; but it's quoted so the `helper' arg ends up unused!
-	      (setq answer '(^ . helper))
+	      (setq answer (cons '^ helper))
 	    (if (or (eq user-sez ?\C-m) (eq user-sez ?\C-j) (eq user-sez ?y))
 		(setq user-sez d-char))
 	    ;; these char-to-int things are because of some

but it would be nice to have someone confirm that this analysis is right
before doing anything.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: feedmail

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Sat, 15 Jun 2019 17:28:13 +0200
> 
> I think the fix is pretty trivial:
> 
> diff --git a/lisp/mail/feedmail.el b/lisp/mail/feedmail.el
> index 943bdd8851..6f50c3e4ed 100644
> --- a/lisp/mail/feedmail.el
> +++ b/lisp/mail/feedmail.el
> @@ -2170,7 +2170,7 @@ feedmail-queue-send-edit-prompt-inner
>  	  (if (= user-sez help-char)
>                ;; FIXME: This seems to want to refer to the `helper' argument,
>                ;; but it's quoted so the `helper' arg ends up unused!
> -	      (setq answer '(^ . helper))
> +	      (setq answer (cons '^ helper))
>  	    (if (or (eq user-sez ?\C-m) (eq user-sez ?\C-j) (eq user-sez ?y))
>  		(setq user-sez d-char))
>  	    ;; these char-to-int things are because of some
> 
> but it would be nice to have someone confirm that this analysis is right
> before doing anything.

I don't think you'll find someone to confirm: feedmail is probably
going to be moved to obsolete/.  So I suggest to push that change.

Re: Towards a cleaner build: feedmail

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't think you'll find someone to confirm: feedmail is probably
> going to be moved to obsolete/.  So I suggest to push that change.

OK; will do.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: tags

[Lars Ingebrigtsen]

27% of the remaining Warning: lines are related to the introduction of
the new xref code, I think:

emulation/viper-ex.el:2019:16:Warning: `find-tag' is an obsolete function (as
    of 25.1); use `xref-find-definitions' instead.
emulation/viper-ex.el:2020:14:Warning: `find-tag-other-window' is an obsolete
    function (as of 25.1); use `xref-find-definitions-other-window' instead.
eshell/esh-mode.el:499:6:Warning: `find-tag' is an obsolete function (as of
    25.1); use `xref-find-definitions' instead.
textmodes/reftex-global.el:101:6:Warning: tags-query-replace called with 4
    arguments, but accepts only 2-3
vc/vc-dir.el:826:4:Warning: tags-query-replace called with 4 arguments, but
    accepts only 2-3

These functions are obsolete (or are called with the wrong number of
arguments), but as far as I can tell, there are no direct substitutions
for these functions (or arguments).

As Emacs 25.1 was a while ago, it would be really nice if somebody were
to write the glue code what would allow us to get rid of these
warnings.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: srecode/insert

[Lars Ingebrigtsen]

As with much in the eieio/CLOS confusion in CEDET, this left me
scratching my head:

cedet/srecode/insert.el:176:39:Warning: Obsolete name arg "TEMPLATE" to
    constructor srecode-template-inserted-region

So the code is:

	     ;; Create the field-driven editable area.
	     (srecode-template-inserted-region
	      "TEMPLATE" :start start :end (point))))

Let's try it:

(srecode-template-inserted-region "TEMPLATE" :start 1 :end 2)
=> #s(srecode-template-inserted-region #<overlay from 1 to 2 in *unsent
wide reply to Lars Ingebrigtsen*> nil)

Is it literally complaining about that first parameter?

(srecode-template-inserted-region :start 1 :end 2)
=> #s(srecode-template-inserted-region #<overlay from 1 to 2 in *unsent wide reply to Lars Ingebrigtsen*> nil)

(equal (srecode-template-inserted-region "TEMPLATE" :start 1 :end 2)
       (srecode-template-inserted-region :start 1 :end 2))
=> t

Indeed?  The name arg is just discarded?  In which case, it seems like
this could be fixed by just removing "TEMPLATE" from all these calls,
but that seems too obvious, so I'm asking. 

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: srecode/insert

[Eric Ludlam]

On 6/15/19 12:33 PM, Lars Ingebrigtsen wrote:

> Is it literally complaining about that first parameter?
> 
> (srecode-template-inserted-region :start 1 :end 2)
> => #s(srecode-template-inserted-region #<overlay from 1 to 2 in *unsent wide reply to Lars Ingebrigtsen*> nil)
> 
> (equal (srecode-template-inserted-region "TEMPLATE" :start 1 :end 2)
>         (srecode-template-inserted-region :start 1 :end 2))
> => t
> 
> Indeed?  The name arg is just discarded?  In which case, it seems like
> this could be fixed by just removing "TEMPLATE" from all these calls,
> but that seems too obvious, so I'm asking.


In old EIEIO, the first argument to a constructor was a name.  I often 
used the name to distinguish between different instances.  That way when 
debugging I could identify which object instance was which.  Sometimes, 
such as above, there was no useful distinguishing name and a constant 
was used.

If I remember rightly, the name isn't part of CLOS, so it was made 
optional, and now appears to be obsoleted.  Since the only value of the 
name in most cases was for debugging, removing it should have no 
functional effect.

The one exception is if the class in question inherits from 
'eieio-named'.  I'm not really sure how that might be made to work 
without the name input, and I don't have a recent enough Emacs to easily 
see how it was adapted.

Hope this helps
Eric

Re: Towards a cleaner build: srecode/insert

[Lars Ingebrigtsen]

Eric Ludlam <eric@siege-engine.com> writes:

> If I remember rightly, the name isn't part of CLOS, so it was made
> optional, and now appears to be obsoleted.  Since the only value of
> the name in most cases was for debugging, removing it should have no
> functional effect.
>
> The one exception is if the class in question inherits from
> 'eieio-named'.  I'm not really sure how that might be made to work
> without the name input, and I don't have a recent enough Emacs to
> easily see how it was adapted.
>
> Hope this helps

Thanks; it does.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: tags

[Dmitry Gutov]

On 15.06.2019 19:11, Lars Ingebrigtsen wrote:
> 27% of the remaining Warning: lines are related to the introduction of
> the new xref code, I think:
> 
> emulation/viper-ex.el:2019:16:Warning: `find-tag' is an obsolete function (as
>      of 25.1); use `xref-find-definitions' instead.
> emulation/viper-ex.el:2020:14:Warning: `find-tag-other-window' is an obsolete
>      function (as of 25.1); use `xref-find-definitions-other-window' instead.
> eshell/esh-mode.el:499:6:Warning: `find-tag' is an obsolete function (as of
>      25.1); use `xref-find-definitions' instead.
> textmodes/reftex-global.el:101:6:Warning: tags-query-replace called with 4
>      arguments, but accepts only 2-3
> vc/vc-dir.el:826:4:Warning: tags-query-replace called with 4 arguments, but
>      accepts only 2-3
> 
> These functions are obsolete (or are called with the wrong number of
> arguments), but as far as I can tell, there are no direct substitutions
> for these functions (or arguments).
> 
> As Emacs 25.1 was a while ago, it would be really nice if somebody were
> to write the glue code what would allow us to get rid of these
> warnings.  :-)

FWIW, I believe tags-query-replace has an adequate replacement already 
in the fileloop package (as you can see it's already implemented in 
terms of it). That would take care of the last two warnings.

esh-mode.el has a peculiar use for find-tag in creating a special 
version of it that "ignores whether the text is read-only". I'm not sure 
why that would matter since the command does not modify the buffer 
contents. Should we just remove the command and the custom binding? Not 
sure if eshell-mode just set up xref-find-functions as well. Is it 
supposed to use a tags table, or find-func.el?

As for viper, I guess the emulation stuff has some UI requrements? 
Otherwise we could use xref-find-definitions and 
xref-find-definitions-other-window there.

Re: Towards a cleaner build: tags

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> FWIW, I believe tags-query-replace has an adequate replacement already
> in the fileloop package (as you can see it's already implemented in
> terms of it). That would take care of the last two warnings.

Thanks; that was straightforward to rewrite...

> esh-mode.el has a peculiar use for find-tag in creating a special
> version of it that "ignores whether the text is read-only". I'm not
> sure why that would matter since the command does not modify the
> buffer contents.

It's kinda puzzling.  The comment in the key binding section is:

  ;; without this, find-tag complains about read-only text being
  ;; modified
  (if (eq (key-binding [(meta ?.)]) 'find-tag)
      (define-key eshell-mode-map [(meta ?.)] 'eshell-find-tag))

Perhaps this was somehow necessary in the year 2000 when it was written? 

> Should we just remove the command and the custom
> binding? Not sure if eshell-mode just set up xref-find-functions as
> well. Is it supposed to use a tags table, or find-func.el?

I can't see any mention of xref in the eshell files...

> As for viper, I guess the emulation stuff has some UI requrements?
> Otherwise we could use xref-find-definitions and
> xref-find-definitions-other-window there.

The latter is fine, but the call for find-tag is with the next-p parameter:

	  (if (string= tag "")
	      (find-tag ex-tag t)
	    (find-tag-other-window ex-tag))

How is that done in terms of xref?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: select.el

[Lars Ingebrigtsen]

There are now 11 compilation Warnings: lines left...  We discussed this
one earlier, but didn't do anything about it:

In xselect--encode-string:
select.el:482:24:Warning: `string-make-unibyte' is an obsolete function (as of
    26.1); use `encode-coding-string'.

It's this code:

	   ((eq type 'C_STRING)
            ;; If STR is unibyte (the normal case), use it; otherwise
            ;; we assume some of the characters are eight-bit, and
            ;; take their lower 8 bits.
	    (setq str (string-make-unibyte str)))

Stefan suggested, I think, that we should just use string-to-unibyte
here.  Eli, do you agree?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: calc.el

[Lars Ingebrigtsen]

This one is really weird:

calc/calc.el:3928:1:Warning: the following functions are not known to be
    defined: math-zerop, math-compare

I'm only getting this on one machine, but not the other.  And calc has
this:

(declare-function math-compare "calc-ext" (a b))
(declare-function math-zerop "calc-misc" (a))

Hm...  One machine compiles a lot faster than the other (and I'm
compiling with -j8), so perhaps there's some ordering issue here?  There
are no calls to math-zerop at all directly in calc.el, but instead
there's the Math-zerop calls:

   (and (Math-zerop a) (not (eq (car-safe a) 'mod))
	(if (and (math-floatp a) (Math-ratp b)) (math-float b) b))

Which are defined in calc-macs, and look like:

;;; Faster in-line version zerop, normalized values only.
(defsubst Math-zerop (a)		; [P N]
  (if (consp a)
      (and (not (memq (car a) '(bigpos bigneg)))
	   (if (eq (car a) 'float)
	       (eq (nth 1 a) 0)
	     (math-zerop a)))
    (eq a 0)))

Uhm...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: calc.el

[Lars Ingebrigtsen]

Here's a minimal reproduction case:

[larsi@stories ~/src/emacs/trunk/lisp]$ rm calc/calc-macs.elc calc/calc.elc; make
make -C ../leim all EMACS="../src/emacs"
make[1]: Entering directory '/home/larsi/src/emacs/trunk/leim'
make[1]: Nothing to be done for 'all'.
make[1]: Leaving directory '/home/larsi/src/emacs/trunk/leim'
make -C ../admin/grammars all EMACS="../../src/emacs"
make[1]: Entering directory '/home/larsi/src/emacs/trunk/admin/grammars'
make[1]: Nothing to be done for 'all'.
make[1]: Leaving directory '/home/larsi/src/emacs/trunk/admin/grammars'
make[1]: Entering directory '/home/larsi/src/emacs/trunk/lisp'
  ELC      calc/calc.elc

In end of data:
calc/calc.el:3928:1:Warning: the following functions are not known to be
    defined: math-zerop, math-compare
  ELC      calc/calc-macs.elc
make[1]: Leaving directory '/home/larsi/src/emacs/trunk/lisp'

And I'm still only seeing this on one machine and not the other...  Is
anybody else seeing this?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: calc.el

[Noam Postavsky]

On Sun, 16 Jun 2019 at 12:11, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> [larsi@stories ~/src/emacs/trunk/lisp]$ rm calc/calc-macs.elc calc/calc.elc; make

>   ELC      calc/calc.elc
>
> In end of data:
> calc/calc.el:3928:1:Warning: the following functions are not known to be
>     defined: math-zerop, math-compare
>   ELC      calc/calc-macs.elc
> make[1]: Leaving directory '/home/larsi/src/emacs/trunk/lisp'
>
> And I'm still only seeing this on one machine and not the other...  Is
> anybody else seeing this?

Happens here too.

Re: Towards a cleaner build: calc.el

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> On Sun, 16 Jun 2019 at 12:11, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
>> [larsi@stories ~/src/emacs/trunk/lisp]$ rm calc/calc-macs.elc calc/calc.elc; make
>
>>   ELC      calc/calc.elc
>>
>> In end of data:
>> calc/calc.el:3928:1:Warning: the following functions are not known to be
>>     defined: math-zerop, math-compare
>>   ELC      calc/calc-macs.elc
>> make[1]: Leaving directory '/home/larsi/src/emacs/trunk/lisp'
>>
>> And I'm still only seeing this on one machine and not the other...  Is
>> anybody else seeing this?
>
> Happens here too.

Most puzzling.  Both calc and calc-macs have declare-function on
math-zerop, but if calc.el pulls in the uncompiled version of Math-zerop
(which then calls math-zerop), then we're getting the warning anyway...
It calc-macs is compiled, this doesn't happen.

We can fix this warning by moving some of these substs around, but it
seems like a bug in the byte-compiler?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: calc.el

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Hm...  One machine compiles a lot faster than the other (and I'm
> compiling with -j8), so perhaps there's some ordering issue here? 

That seems to be the case.  On the machine I'm getting the warning, the
log is

  ELC      calc/calc.elc
  ELC      calc/calc-embed.elc

[...]

  ELC      calc/calc-lang.elc
  ELC      calc/calc-macs.elc

and the one with no warnings it's

  ELC      calc/calc-macs.elc
  ELC      calc/calc-map.elc

[...]

  ELC      calc/calc-yank.elc
  ELC      calc/calc.elc

and when calc-macs is compiled before calc, then there's no warning.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: calc.el

[Stefan Monnier]

> ;;; Faster in-line version zerop, normalized values only.
> (defsubst Math-zerop (a)		; [P N]
>   (if (consp a)
>       (and (not (memq (car a) '(bigpos bigneg)))
> 	   (if (eq (car a) 'float)
> 	       (eq (nth 1 a) 0)
> 	     (math-zerop a)))
>     (eq a 0)))

There's indeed an ordering issue with defsubst:
when compiling a call to a defsubst such as Math-zerop, if Math-zerop is
already compiled, then we inline its bytecode (this is done at the level
of the "lapcode"), whereas if it's not compiled yet, then we inline the
source code directly.

Of course, to make things more interesting, this is only half-true: if
the lexical-binding of the two files is different, then even if the
function is not yet compiled, we don't inline its source code, instead
we first compile it and then inline its bytecode (to avoid having to
compile code that's a mix of lexical-binding and dynamic-binding).

Arguably, we should drop support for inlining the source code and just
always byte-compile the function before inlining it to avoid those
side-effects.
See byte-compile-inline-expand if you're interested in this.


        Stefan

Re: Towards a cleaner build: calc.el

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Of course, to make things more interesting, this is only half-true: if
> the lexical-binding of the two files is different, then even if the
> function is not yet compiled, we don't inline its source code, instead
> we first compile it and then inline its bytecode (to avoid having to
> compile code that's a mix of lexical-binding and dynamic-binding).

Hm...  So that made me wonder what would happen if calc-macs was
lexical:

[larsi@stories ~/src/emacs/trunk/lisp]$ rm calc/calc-macs.elc calc/calc.elc; make

[...]

make[1]: Entering directory '/home/larsi/src/emacs/trunk/lisp'
  ELC      calc/calc.elc
  ELC      calc/calc-macs.elc
make[1]: Leaving directory '/home/larsi/src/emacs/trunk/lisp'

Well, that was easy?  :-)

calc-tests passes, too...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: calc.el

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> make[1]: Entering directory '/home/larsi/src/emacs/trunk/lisp'
>   ELC      calc/calc.elc
>   ELC      calc/calc-macs.elc
> make[1]: Leaving directory '/home/larsi/src/emacs/trunk/lisp'
>
> Well, that was easy?  :-)
>
> calc-tests passes, too...

I had a look at the file, and making that file lexical seems
straight-forward.  That is, no changes were needed, and the tests pass
(but I haven't checked whether there's really any coverage), but M-x
calc works afterwards, so I went ahead and added the -*- lexical.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: calc.el

[Mattias Engdegård]

17 juni 2019 kl. 12.47 skrev Lars Ingebrigtsen <larsi@gnus.org>:
> 
>> make[1]: Entering directory '/home/larsi/src/emacs/trunk/lisp'
>>  ELC      calc/calc.elc
>>  ELC      calc/calc-macs.elc
>> make[1]: Leaving directory '/home/larsi/src/emacs/trunk/lisp'
>> 
>> Well, that was easy?  :-)
>> 
>> calc-tests passes, too...
> 
> I had a look at the file, and making that file lexical seems
> straight-forward.  That is, no changes were needed, and the tests pass
> (but I haven't checked whether there's really any coverage), but M-x
> calc works afterwards, so I went ahead and added the -*- lexical.

The coverage of calc-tests is practically infinitesimal, but calc-macs.el is probably safe with lexical-binding.

Re: Towards a cleaner build: select.el

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Sun, 16 Jun 2019 16:45:20 +0200
> 
> In xselect--encode-string:
> select.el:482:24:Warning: `string-make-unibyte' is an obsolete function (as of
>     26.1); use `encode-coding-string'.
> 
> It's this code:
> 
> 	   ((eq type 'C_STRING)
>             ;; If STR is unibyte (the normal case), use it; otherwise
>             ;; we assume some of the characters are eight-bit, and
>             ;; take their lower 8 bits.
> 	    (setq str (string-make-unibyte str)))
> 
> Stefan suggested, I think, that we should just use string-to-unibyte
> here.  Eli, do you agree?

Sorry, no, at least not yet.  I still cannot wrap my head around that
code, and Kenichi Handa didn't respond to my questions.  Please leave
this alone for now, it's still on my todo.

Re: Towards a cleaner build: select.el

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Sorry, no, at least not yet.  I still cannot wrap my head around that
> code, and Kenichi Handa didn't respond to my questions.  Please leave
> this alone for now, it's still on my todo.

Okidoke.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: tags

[Dmitry Gutov]

On 16.06.2019 16:20, Lars Ingebrigtsen wrote:

>> esh-mode.el has a peculiar use for find-tag in creating a special
>> version of it that "ignores whether the text is read-only". I'm not
>> sure why that would matter since the command does not modify the
>> buffer contents.
> 
> It's kinda puzzling.  The comment in the key binding section is:
> 
>    ;; without this, find-tag complains about read-only text being
>    ;; modified
>    (if (eq (key-binding [(meta ?.)]) 'find-tag)
>        (define-key eshell-mode-map [(meta ?.)] 'eshell-find-tag))
> 
> Perhaps this was somehow necessary in the year 2000 when it was written?

Perhaps. No idea. :(

>> Should we just remove the command and the custom
>> binding? Not sure if eshell-mode just set up xref-find-functions as
>> well. Is it supposed to use a tags table, or find-func.el?
> 
> I can't see any mention of xref in the eshell files...

Sorry, I meant: Not sure if eshell-mode *should* set up 
xref-find-functions as well.

>> As for viper, I guess the emulation stuff has some UI requrements?
>> Otherwise we could use xref-find-definitions and
>> xref-find-definitions-other-window there.
> 
> The latter is fine, but the call for find-tag is with the next-p parameter:
> 
> 	  (if (string= tag "")
> 	      (find-tag ex-tag t)
> 	    (find-tag-other-window ex-tag))
> 
> How is that done in terms of xref?

But what does NEXT-P mean in this context? It's not like there has been 
a previous invocation of find-tag, was there?

BTW, your last change in this function is clearly inadequate: find-tag 
and xref-find-definitions-other-window present different UIs when 
there's more locations than one. We shouldn't use both in the same command.

Re: Towards a cleaner build: tags

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

>>> Should we just remove the command and the custom
>>> binding? Not sure if eshell-mode just set up xref-find-functions as
>>> well. Is it supposed to use a tags table, or find-func.el?
>>
>> I can't see any mention of xref in the eshell files...
>
> Sorry, I meant: Not sure if eshell-mode *should* set up
> xref-find-functions as well.

If it should, how does it do that?

>>> As for viper, I guess the emulation stuff has some UI requrements?
>>> Otherwise we could use xref-find-definitions and
>>> xref-find-definitions-other-window there.
>>
>> The latter is fine, but the call for find-tag is with the next-p parameter:
>>
>> 	  (if (string= tag "")
>> 	      (find-tag ex-tag t)
>> 	    (find-tag-other-window ex-tag))
>>
>> How is that done in terms of xref?
>
> But what does NEXT-P mean in this context? It's not like there has
> been a previous invocation of find-tag, was there?

It seems like the logic is that the first time the function is called,
find-tag-other-window, and the next one it's calling find-tag with
NEXT-P?

> BTW, your last change in this function is clearly inadequate: find-tag
> and xref-find-definitions-other-window present different UIs when
> there's more locations than one. We shouldn't use both in the same
> command.

Ah; I'll revert that patch, then.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: tags

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>>> The latter is fine, but the call for find-tag is with the next-p parameter:
>>>
>>> 	  (if (string= tag "")
>>> 	      (find-tag ex-tag t)
>>> 	    (find-tag-other-window ex-tag))
>>>
>>> How is that done in terms of xref?
>>
>> But what does NEXT-P mean in this context? It's not like there has
>> been a previous invocation of find-tag, was there?
>
> It seems like the logic is that the first time the function is called,
> find-tag-other-window, and the next one it's calling find-tag with
> NEXT-P?

Yes, that seems to be the logic.

But does xref provide a command to jump to the next match without going
back to the *xref* window at all?  I looked through the code, and I
couldn't find it...

Oh, it's just `next-error'?  Well, that makes sense.  I'll just rewrite
the code to be xref-find-tag-other-window/next-error.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: tags

[Dmitry Gutov]

On 20.06.2019 15:00, Lars Ingebrigtsen wrote:
> But does xref provide a command to jump to the next match without going
> back to the*xref*  window at all?  I looked through the code, and I
> couldn't find it...

No command. Of course, you could use (with-current-buffer ...), I guess. 
Or something like that.

> Oh, it's just `next-error'?  Well, that makes sense.  I'll just rewrite
> the code to be xref-find-tag-other-window/next-error.

That will probably work. Might be a tad unreliable, but that's just my 
intuition.

Re: Towards a cleaner build: tags

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> Oh, it's just `next-error'?  Well, that makes sense.  I'll just rewrite
>> the code to be xref-find-tag-other-window/next-error.
>
> That will probably work. Might be a tad unreliable, but that's just my
> intuition.

Probably not all that much unreliable than the code already was.  I
think it's basically that if you're on a symbol, then :tag takes you to
the definition, and then you have to move off the symbol definition and
issue :tag again to go to the next definition...  but it means that if
you hadn't done the first thing already, then the second thing wouldn't
work anyway.

At least that's my reading of the code.  

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: tags

[Dmitry Gutov]

On 19.06.2019 16:18, Lars Ingebrigtsen wrote:
>>>> Should we just remove the command and the custom
>>>> binding? Not sure if eshell-mode just set up xref-find-functions as
>>>> well. Is it supposed to use a tags table, or find-func.el?
>>> I can't see any mention of xref in the eshell files...
>> Sorry, I meant: Not sure if eshell-mode*should*  set up
>> xref-find-functions as well.
> If it should, how does it do that?

By copying a line from emacs-lisp-mode, for instance. Or writing 
something custom, if the intent was to jump to something other than 
currently loaded Lisp functions/variables/etc.

(I meant xref-backend-functions though. Sorry.)

Re: Towards a cleaner build: tags

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> If it should, how does it do that?
>
> By copying a line from emacs-lisp-mode, for instance. Or writing
> something custom, if the intent was to jump to something other than
> currently loaded Lisp functions/variables/etc.
>
> (I meant xref-backend-functions though. Sorry.)

I think it was probably just legacy code, so I obsoleted it and restored
the default M-. binding in esh buffers.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: eieio

[Stefan Monnier]

> Sorry, I meant -- are all these object-print methods supposed to be
> expressed as cl-print-object methods instead?

I think so.

> And if so, can they be rewritten that way now without breaking anything?

I think so too (tho they'll only work that way with Emacs>26, of course).

> Or would that break out-of-tree code that calls object-print?

There's also a risk of that, of course.

> eieio.el has general fallback code for object-print, but does not
> consult the output of cl-print-object for the objects, so I guess that
> might break something?  But...  could we do that, without going all
> recursive?

Right, I don't think we want to try and make object-print fallback to
cl-print-object: not worth the trouble.


        Stefan

Re: Towards a cleaner build: eieio

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Sorry, I meant -- are all these object-print methods supposed to be
>> expressed as cl-print-object methods instead?
>
> I think so.
>
>> And if so, can they be rewritten that way now without breaking anything?
>
> I think so too (tho they'll only work that way with Emacs>26, of course).

OK; I'll take a look at rewriting one of them to see how much work it
is...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: eieio

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> OK; I'll take a look at rewriting one of them to see how much work it
> is...

OK, here's my test case:

(require 'semantic/db-el)
(setq obj (semanticdb-table-emacs-lisp :major-mode "foo"))

(object-print obj)
=> "#<semanticdb-table-emacs-lisp semanticdb-table-emacs-lisp-15884632da6c (proxy)>"

(cl-print-object obj (current-buffer))
#<semanticdb-table-emacs-lisp semanticdb-table-emacs-lisp-15884632da6c (proxy)>

So that's the way it is with an object-print method on the object.  If I
apply the following patch, I then get identical output:

(cl-print-object obj (current-buffer))
#<semanticdb-table-emacs-lisp semanticdb-table-emacs-lisp-15884632da6c (proxy)>

That's a kind of odd object-print, though, because it contains so little
information.  But I've followed the call-next-method chain up, and
there's nothing added, so I think this change is correct?  (Because the
optional STRINGS is always nil; i.e., there are no actual direct calls
to object-print.)

If this looks reasonable to you, I can do the transforms for the other
five instances of object-print in cedet.

diff --git a/lisp/cedet/semantic/db-el.el b/lisp/cedet/semantic/db-el.el
index 5375623c13..b30553d95b 100644
--- a/lisp/cedet/semantic/db-el.el
+++ b/lisp/cedet/semantic/db-el.el
@@ -53,10 +53,9 @@ semanticdb-needs-refresh-p
   "Return nil, we never need a refresh."
   nil)
 
-(cl-defmethod object-print ((obj semanticdb-table-emacs-lisp) &rest strings)
-  "Pretty printer extension for `semanticdb-table-emacs-lisp'.
-Adds the number of tags in this file to the object print name."
-  (apply #'cl-call-next-method obj (cons " (proxy)" strings)))
+(cl-defmethod cl-print-object ((obj semanticdb-table-emacs-lisp) stream)
+  "Pretty printer extension for `semanticdb-table-emacs-lisp'."
+  (princ (eieio-object-name obj " (proxy)") stream))
 
 (defclass semanticdb-project-database-emacs-lisp
   (semanticdb-project-database eieio-singleton)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: eieio

[Stefan Monnier]

> So that's the way it is with an object-print method on the object.  If I
> apply the following patch, I then get identical output:
>
> (cl-print-object obj (current-buffer))
> #<semanticdb-table-emacs-lisp semanticdb-table-emacs-lisp-15884632da6c (proxy)>
[...]
> +(cl-defmethod cl-print-object ((obj semanticdb-table-emacs-lisp) stream)
> +  "Pretty printer extension for `semanticdb-table-emacs-lisp'."
> +  (princ (eieio-object-name obj " (proxy)") stream))

Hmm... how does this code result in

    #<semanticdb-table-emacs-lisp semanticdb-table-emacs-lisp-15884632da6c (proxy)>

?

This said, I'm not sure why we should care very much about preserving
the detailed output in this particular case, since I don't think the
rest of code will ever do much with it (it's only for human consumption
and it's not particularly good at it).


        Stefan

Re: Towards a cleaner build: eieio

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> +(cl-defmethod cl-print-object ((obj semanticdb-table-emacs-lisp) stream)
>> +  "Pretty printer extension for `semanticdb-table-emacs-lisp'."
>> +  (princ (eieio-object-name obj " (proxy)") stream))
>
> Hmm... how does this code result in
>
>     #<semanticdb-table-emacs-lisp semanticdb-table-emacs-lisp-15884632da6c (proxy)>

(eieio-object-name obj)
=> "#<semanticdb-table-emacs-lisp semanticdb-table-emacs-lisp-15884632da6c>"

and it just appends whatever string you give to it after the object...

> This said, I'm not sure why we should care very much about preserving
> the detailed output in this particular case, since I don't think the
> rest of code will ever do much with it (it's only for human consumption
> and it's not particularly good at it).

No, we probably don't care much in this particular instance, but it was
a really simple one to use as a test case.

If you otherwise think it looks like a sensible transform, I can start
doing the other ones (and some of them have better printed
representations, if I recall correctly from skimming them).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: eieio

[Stefan Monnier]

>> Hmm... how does this code result in
>>
>>     #<semanticdb-table-emacs-lisp semanticdb-table-emacs-lisp-15884632da6c (proxy)>
>
> (eieio-object-name obj)
> => "#<semanticdb-table-emacs-lisp semanticdb-table-emacs-lisp-15884632da6c>"
>
> and it just appends whatever string you give to it after the object...

Ha!  Cool!

>> This said, I'm not sure why we should care very much about preserving
>> the detailed output in this particular case, since I don't think the
>> rest of code will ever do much with it (it's only for human consumption
>> and it's not particularly good at it).
>
> No, we probably don't care much in this particular instance, but it was
> a really simple one to use as a test case.

LGTM,


        Stefan

Re: Towards a cleaner build: eieio

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> No, we probably don't care much in this particular instance, but it was
>> a really simple one to use as a test case.
>
> LGTM,

I've now rewritten the object-prints to be cl-print-objects (and had to
introduce a new method to get the output to be identical (probably not
very important, but...)), so I thought I was going to go ahead and
suppress this fallback method in eieio.el:

In toplevel form:
emacs-lisp/eieio.el:857:11:Warning: `object-print' is an obsolete function (as
    of 26.1); use `cl-print-object' instead.

But!  Somehow with-suppressed-warnings doesn't work in methods?

(cl-defmethod cl-print-object ((object eieio-default-superclass) stream)
  "Default printer for EIEIO objects."
  ;; Fallback to the old `object-print'.
  (princ (object-print object) stream))

I can work around this trivially, but it would be nice if that macro
worked here, too.  Are cl-defmethods compiled in such a way that the
bytecomp macrology magic is...  done in a different order so that that
doesn't work?

Hm...  with-no-warnings works...  *scratches head*

Oh!  This patch was needed to get that macro working here, and Emacs had
to be rebuilt for it to happen:

diff --git a/lisp/emacs-lisp/cl-generic.el b/lisp/emacs-lisp/cl-generic.el
index 9013c1f060..10190f4933 100644
--- a/lisp/emacs-lisp/cl-generic.el
+++ b/lisp/emacs-lisp/cl-generic.el
@@ -450,7 +450,7 @@ cl-defmethod
       `(progn
          ,(and (get name 'byte-obsolete-info)
                (or (not (fboundp 'byte-compile-warning-enabled-p))
-                   (byte-compile-warning-enabled-p 'obsolete))
+                   (byte-compile-warning-enabled-p 'obsolete name))
                (let* ((obsolete (get name 'byte-obsolete-info)))
                  (macroexp--warn-and-return
                   (macroexp--obsolete-warning name obsolete "generic function")


Now done.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: eieio

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Hm...  with-no-warnings works...  *scratches head*
>
> Oh!  This patch was needed to get that macro working here, and Emacs had
> to be rebuilt for it to happen:

Uhm, no, that was not correct.  This warning is output by cl-generic on
the definition of the method, of course:

  ELC      emacs-lisp/eieio.elc
`object-print' is an obsolete generic function (as of 26.1); use `cl-print-object' instead.

But with-suppressed-warnings does not work in the body of the method,
while with-no-warnings does, which is rather puzzling...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: eieio

[Lars Ingebrigtsen]

In eieio-object-value-get:
emacs-lisp/eieio-custom.el:320:15:Warning: `eieio-object-set-name-string' is
    an obsolete function (as of 25.1); inherit from `eieio-named' and use
    (setf (slot-value OBJ 'object-name) NAME) instead

It's this code:

(defun eieio-object-value-get (widget)
  "Get the value of WIDGET."
  (let* ((obj (widget-get widget :value))

[...]

    ;; Set any name updates on it.
    (if name (eieio-object-set-name-string obj name))

Virtually all of the cedet objects can be customised, so this would
require making them all inherit from eieio-named...  But I wonder
whether this code makes that much sense?  Why would anybody, in a
Customize buffer, alter the name of the objects?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: eieio

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Virtually all of the cedet objects can be customised, so this would
> require making them all inherit from eieio-named... 

Actually, that was inaccurate.  It's only the ede objects, not
everything in cedet.  So by making ede-target inherit from eieio-named,
I think this can be fixed easily...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: ieieo

[Lars Ingebrigtsen]

But there's more funny stuff going on in eieio:

make[1]: Entering directory '/home/larsi/src/emacs/trunk/lisp'
  ELC      emacs-lisp/eieio.elc
`object-print' is an obsolete generic function (as of 26.1); use `cl-print-object' instead.

In toplevel form:
emacs-lisp/eieio.el:850:11:Warning: `object-print' is an obsolete function (as
    of 26.1); use `cl-print-object' instead.

I didn't understand where the first message came from for a while, but
it's here:

(defmacro cl-defmethod (name args &rest body)
  "Define a new method for generic function NAME.

[...]

    (pcase-let* ((`(,uses-cnm . ,fun) (cl--generic-lambda args body)))
      `(progn
         ,(and (get name 'byte-obsolete-info)
               (or (not (fboundp 'byte-compile-warning-enabled-p))
                   (byte-compile-warning-enabled-p 'obsolete))
               (let* ((obsolete (get name 'byte-obsolete-info)))
                 (macroexp--warn-and-return
                  (macroexp--obsolete-warning name obsolete "generic function")
                  nil)))

Is this useful?  I mean, the byte compiler does warn about this later
(and in the correct format), so this seems kinda superfluous?  Or is it
also messages outside of byte-compilation?  If that's the case, should
it be suppressed when we're compiling, and if so, how?  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: ieieo

[Stefan Monnier]

>     (pcase-let* ((`(,uses-cnm . ,fun) (cl--generic-lambda args body)))
>       `(progn
>          ,(and (get name 'byte-obsolete-info)
>                (or (not (fboundp 'byte-compile-warning-enabled-p))
>                    (byte-compile-warning-enabled-p 'obsolete))
>                (let* ((obsolete (get name 'byte-obsolete-info)))
>                  (macroexp--warn-and-return
>                   (macroexp--obsolete-warning name obsolete "generic function")
>                   nil)))

IIUC, this is to emit a warning when a package defines a method for an
obsolete generic-function (e.g. a method for object-print) so that the
author is made aware of the need to port the method to the new
generic function (e.g. cl-print-object).


        Stefan

Re: Towards a cleaner build: ieieo

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>     (pcase-let* ((`(,uses-cnm . ,fun) (cl--generic-lambda args body)))
>>       `(progn
>>          ,(and (get name 'byte-obsolete-info)
>>                (or (not (fboundp 'byte-compile-warning-enabled-p))
>>                    (byte-compile-warning-enabled-p 'obsolete))
>>                (let* ((obsolete (get name 'byte-obsolete-info)))
>>                  (macroexp--warn-and-return
>>                   (macroexp--obsolete-warning name obsolete "generic function")
>>                   nil)))
>
> IIUC, this is to emit a warning when a package defines a method for an
> obsolete generic-function (e.g. a method for object-print) so that the
> author is made aware of the need to port the method to the new
> generic function (e.g. cl-print-object).

Yes, but the byte compiler will also output this warning...  so you get
the warning twice, but only the second is on a form that allows jumping
to the definition:

`object-print' is an obsolete generic function (as of 26.1); use `cl-print-object' instead.

In toplevel form:
emacs-lisp/eieio.el:850:11:Warning: `object-print' is an obsolete function (as
    of 26.1); use `cl-print-object' instead.


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: ieieo

[Stefan Monnier]

> `object-print' is an obsolete generic function (as of 26.1); use `cl-print-object' instead.

This warning is for

    (cl-defmethod object-print ((this eieio-default-superclass) &rest strings)

> In toplevel form:
> emacs-lisp/eieio.el:850:11:Warning: `object-print' is an obsolete function (as
>     of 26.1); use `cl-print-object' instead.

This one is for

      (princ (object-print object) stream))


-- Stefan

Re: Towards a cleaner build: ieieo

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> `object-print' is an obsolete generic function (as of 26.1); use
>> `cl-print-object' instead.
>
> This warning is for
>
>     (cl-defmethod object-print ((this eieio-default-superclass) &rest strings)
>
>> In toplevel form:
>> emacs-lisp/eieio.el:850:11:Warning: `object-print' is an obsolete function (as
>>     of 26.1); use `cl-print-object' instead.
>
> This one is for
>
>       (princ (object-print object) stream))

Ah, I see.  Thanks.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

string-to-multibyte and string-make-*byte (was: Towards a cleaner build)

[Stefan Monnier]

>>>> I think, just as for string-to-unibyte, we likely should un-obsolete
>>>> string-to-multibyte.
>>> Yes, I think so too -- they are sometimes what one needs to do...
>> This has been mooted a few times in the past, too, and nobody has
>> protested.  So I'll go ahead and remove the obsoletions of those two
>> functions (and note it in NEWS).
> Oh, I missed that string-to-unibyte had already been unobsoleted, but
> string-to-multibyte hadn't.  So perhaps there were objections to that
> part that I missed?

No, I think when string-to-unibyte was de-obsoleted the other one just
didn't come up, but I don't think there's a string objection.

While I'm here, I'd suggest that we turn string-make-*byte into obsolete
aliases for string-to-*byte (their specific behavior has always been
heuristic in nature, and we changed `insert` to implicitly do
string-to-*byte instead of string-make-*byte when needed, so it seems
natural to do that).


        Stefan

Re: string-to-multibyte and string-make-*byte

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> No, I think when string-to-unibyte was de-obsoleted the other one just
> didn't come up, but I don't think there's a string objection.

Oh, OK, I'll unobsolete string-to-multibyte too, then.

> While I'm here, I'd suggest that we turn string-make-*byte into obsolete
> aliases for string-to-*byte (their specific behavior has always been
> heuristic in nature, and we changed `insert` to implicitly do
> string-to-*byte instead of string-make-*byte when needed, so it seems
> natural to do that).

Sounds good to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: pcomplete

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> This one I'm not sure what the status is with: It's an obsolete
>> variable, but it seems unsure how it should be replaced here,
>
> Right, it should be replaced by obsoleting the `pcomplete` command (not
> the package).
>
>> In pcomplete/tar:
>> pcmpl-gnu.el:162:47:Warning: `pcomplete-suffix-list' is an obsolete variable
>>     (as of 24.1).
>>
>> I'm not sure whether suppressing this warning is the right thing,
>> either...
>
> Agreed.

Actually, looking at the code further, I don't quite understand this one
at all...  These functions are the ones that are called when you hit TAB
in M-x shell buffers (and the like)?  So "tar --v<TAB>" will complete
over the completions like "--verbose"?

And I guess pcomplete is one of these functions that can be used here,
but it's not used by default; right.

But the only place where pcomplete-suffix-alist is actually used for
anything is in pcomplete-insert-entry, and this is the call trace for
that:

Debugger entered: nil
  pcomplete-insert-entry(#("--rsh-comma" 0 11 (fontified t face
  (and result (not (eq (car result) 'listed)) (cdr result) (pco
  (let* ((pcomplete-stub) pcomplete-seen pcomplete-norm-func pc
  (catch 'pcompleted (let* ((pcomplete-stub) pcomplete-seen pco
  (if (and interactively pcomplete-cycle-completions pcomplete-
  pcomplete(1)
  funcall-interactively(pcomplete 1)
  call-interactively(pcomplete record nil)
  command-execute(pcomplete record)
  execute-extended-command(nil "pcomplete" nil)
  funcall-interactively(execute-extended-command nil "pcomplete" nil)
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)

And, as we can see, that function is not called from pcomplete/tar, so
the binding of pcomplete-insert-entry in pcomplete/tar doesn't seem like
it could possibly affect anything?

Am I misreading the code?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: pcomplete

[Stefan Monnier]

>>> In pcomplete/tar:
>>> pcmpl-gnu.el:162:47:Warning: `pcomplete-suffix-list' is an obsolete variable
>>>     (as of 24.1).
[...]
> Actually, looking at the code further, I don't quite understand this one
> at all...  These functions are the ones that are called when you hit TAB
> in M-x shell buffers (and the like)?  So "tar --v<TAB>" will complete
> over the completions like "--verbose"?

Right.

> And, as we can see, that function is not called from pcomplete/tar, so
> the binding of pcomplete-insert-entry in pcomplete/tar doesn't seem like
> it could possibly affect anything?

I think you're right: this binding is likely the result of
a misunderstanding (contrary to the setting of pcomplete-suffix-list in
lisp/eshell/em-cmpl.el).


        Stefan

Re: Towards a cleaner build: pcomplete

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Am I misreading the code?

Nope.  A change in 2011 changed

-    (setq pcomplete-suffix-list (cons ?= pcomplete-suffix-list))

to

+    (let ((pcomplete-suffix-list (cons ?= pcomplete-suffix-list)))

But the pcomplete function binds that variable, so the call to
pcomplete/tar alters the value, and then when the pcomplete function
later calls pcomplete-insert-entry, it does the right thing.

So this has been buggy since 2011.  :-)

The bug is that when you call M-x pcomplete now, 

tar --rsh-command= <point ends here>

but with the setq it's:

tar --rsh-command=<point ends here>

It's the same with the standard completion function -- there's a space
after the = sign after hitting TAB, which is a bug...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

We've only got four Warning: lines left, but there's some other
irregular stuff being output, and I wonder whether we could reformat or
remove or fix some of these things.  (The point here is, of course, that
when you're working on Emacs and then compile it, if you see something
that looks like an error you know you've done something wrong.)

There's 38 lines on stderr, so perhaps that's a good place to start.

eieio.el: ‘object-print’ is an obsolete generic function (as of 26.1); use ‘cl-print-object’ instead.

These are output by cl-defmethod/cl-defgeneric, and we're defining an
(obsolete) fallback method, so we should suppress this warning.  Do we
need a new mechanism or do we have something to cover this?

---

*Warning* default value of <<no-type>> tokens changed to QUERY, was NULL_TOKEN
*Warning* default value of <<no-type>> tokens changed to TRUE, was QUERY
*Warning* default value of <<no-type>> tokens changed to FALSE, was TRUE
Grammar in javascript.el contains 2 useless nonterminals and 4 useless rules
Grammar in js-wy.el contains 2 useless nonterminals and 4 useless rules
Grammar in python-wy.el contains 2 useless nonterminals and 3 useless rules
Grammar in python-wy.el contains 4 shift/reduce conflicts
Grammar in python.el contains 2 useless nonterminals and 3 useless rules
Grammar in python.el contains 4 shift/reduce conflicts

These are from cedet, and are warning about grammars containing...
useless rules?  Perhaps this is more useful at run-time than at compile
time?

---

Loading /home/larsi/src/emacs/trunk/lisp/international/uni-name.el (source)...

Hm...  a "Loading" in the middle here?  Does anybody know why?

---

10000 entries
20000 entries
30000 entries
40000 entries
50000 entries
60000 entries

I thought these weren't very useful because they zipped by on my screen,
but people protested.  Could we perhaps reformat them so they look more
like other compilation output:

  ELC      vc/vc-svn.elc
  ELC      progmodes/cc-defs.elc
  GEN      10000 entries
  GEN      20000 entries

Or something?

---

latin-ltx: 1535 rules (+ 0 conflicts)!

Uhm...  That doesn't seem very useful to me -- only output that if there
are any conflicts?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

I think that desperate efforts to remove warnings at build time
are not worth the trouble.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Towards a cleaner build: other output

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Date: Mon, 17 Jun 2019 12:04:13 -0400
> Cc: emacs-devel@gnu.org
> 
> I think that desperate efforts to remove warnings at build time
> are not worth the trouble.

If we had a few of them, I might agree.  But we had gobs of them, and
that frequently made real problems very hard to spot during a build.
So I personally am very grateful to Lars for taking up this boring job
and doing it so well.

Re: Towards a cleaner build: other output

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > I think that desperate efforts to remove warnings at build time
  > > are not worth the trouble.

  > If we had a few of them, I might agree.

Didn't he just say that only a few build warnings remain?

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Richard Stallman <rms@gnu.org> writes:

>   > > I think that desperate efforts to remove warnings at build time
>   > > are not worth the trouble.
>
>   > If we had a few of them, I might agree.
>
> Didn't he just say that only a few build warnings remain?

I'm not quite sure what your objection is here.  There are no "desperate
efforts" taken, and the few remaining build warnings under discussion
are no more complicated to handle than the ones already taken care of.
Instead they're mostly pretty trivial.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Stefan Monnier]

> I'm not quite sure what your objection is here.  There are no "desperate
> efforts" taken, and the few remaining build warnings under discussion
> are no more complicated to handle than the ones already taken care of.
> Instead they're mostly pretty trivial.

I think beyond the question of whether those things deserve to be
silenced in our build, this is also useful to make sure warnings *can*
be silenced cleanly when needed, which I think is an important property
(since all warnings can be false positives).


        Stefan

Re: Towards a cleaner build: other output

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > I'm not quite sure what your objection is here.

I wouldn't call it an "objection".  The point is that we don't _need_
to make zero warnings our target; we don't need to treat every last
warning as a real problem simply because it is a warning.

To have just five warnings is nearly as good as having zero warnings.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> eieio.el: ‘object-print’ is an obsolete generic function (as of 26.1);
> use ‘cl-print-object’ instead.
>
> These are output by cl-defmethod/cl-defgeneric, and we're defining an
> (obsolete) fallback method, so we should suppress this warning.  Do we
> need a new mechanism or do we have something to cover this?

I found a simple work-around for this -- just obsolete the method later
in the file instead of before defining the method.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

There's one point I'd argue for more messaging than we have today.  Or
at least different messaging:

  GEN      loaddefs.el
Not registering prefix "lo" from completion.  Affects: ("locate-completion-entry" "locate-completion-entry-retry" "locate-completion-db-error" "load-completions-from-file")
Not registering prefix "*" from ielm.  Affects: ("*" "**" "***" "*1" "*2" "*3")
(etc)

When we come to this point in the build process, even on my fastest
machine, it pauses for many seconds.  I'm not sure that the "not
registering" thing is useful, but it'd be nice if it gave more feedback
to let us know that it was working on something and not having choked...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

OK, with the patch below I now get

  GEN      loaddefs.el
Not registering prefix "lo" from completion.  Affects: ("locate-completion-entry" "locate-completion-entry-retry" "locate-completion-db-error" "load-completions-from-file")
  INFO     Processed 100 completions from files
Not registering prefix "*" from ielm.  Affects: ("*" "**" "***" "*1" "*2" "*3")
  INFO     Processed 200 completions from files
  INFO     Processed 300 completions from files
  INFO     Processed 400 completions from files
  INFO     Processed 500 completions from files
  INFO     Processed 600 completions from files
  INFO     Processed 700 completions from files
  INFO     Processed 800 completions from files
Not registering prefix "is" from ind-util.  Affects: ("is13194-default-repertory" "is13194-repertory-to-ucs-script" "is13194-to-ucs-devanagari-hashtbl" "is13194-to-ucs-devanagari-regexp" "is13194-to-ucs-bengali-hashtbl" "is13194-to-ucs-bengali-regexp" "is13194-to-ucs-assamese-hashtbl" "is13194-to-ucs-assamese-regexp" "is13194-to-ucs-gurmukhi-hashtbl" "is13194-to-ucs-gurmukhi-regexp" "is13194-to-ucs-gujarati-hashtbl" "is13194-to-ucs-gujarati-regexp" "is13194-to-ucs-oriya-hashtbl" "is13194-to-ucs-oriya-regexp" "is13194-to-ucs-tamil-hashtbl" "is13194-to-ucs-tamil-regexp" "is13194-to-ucs-telugu-hashtbl" "is13194-to-ucs-telugu-regexp" "is13194-to-ucs-malayalam-hashtbl" "is13194-to-ucs-malayalam-regexp" "is13194-to-ucs-kannada-hashtbl" "is13194-to-ucs-kannada-regexp" "iscii-to-ucs-region")
  INFO     Processed 900 completions from files
  INFO     Processed 1000 completions from files
  INFO     Processed 1100 completions from files
  INFO     Processed 1200 completions from files
  INFO     Processed 1300 completions from files
Not registering prefix "se" from page-ext.  Affects: ("search-pages" "set-page-delimiter")
  INFO     Processed 1400 completions from files
make[3]: Leaving directory '/home/larsi/src/emacs/trunk/lisp'
  GEN      ../etc/DOC

This part of the compilation takes about 30 seconds on my faster
machine.

I don't see much value in the "Not registering" messages, because I
don't see how any developer would respond to the output, but I may be
totally wrong here.

Does the INFO thing look reasonable?

diff --git a/lisp/emacs-lisp/autoload.el b/lisp/emacs-lisp/autoload.el
index 19e1e93621..8c9c53c6e5 100644
--- a/lisp/emacs-lisp/autoload.el
+++ b/lisp/emacs-lisp/autoload.el
@@ -1061,6 +1061,7 @@ update-directory-autoloads
          ;; Files with no autoload cookies or whose autoloads go to other
          ;; files because of file-local autoload-generated-file settings.
 	 (no-autoloads nil)
+         (file-count 0)
          (autoload-modified-buffers nil)
 	 (generated-autoload-file
 	  (if (called-interactively-p 'interactive)
@@ -1126,6 +1127,10 @@ update-directory-autoloads
       ;; Elements remaining in FILES have no existing autoload sections yet.
       (let ((no-autoloads-time (or last-time '(0 0 0 0))) file-time)
 	(dolist (file files)
+          (setq file-count (1+ file-count))
+          (when (zerop (mod file-count 100))
+            (byte-compile-info-message "Processed %d completions from files"
+                                       file-count))
 	  (cond
 	   ;; Passing nil as second argument forces
 	   ;; autoload-generate-file-autoloads to look for the right
diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index 3a2043bad7..30a9f98479 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -540,6 +540,10 @@ byte-run--unescaped-character-literals-warning
                         (mapconcat (lambda (char) (format "`?\\%c'" char))
                                    sorted ", ")))))
 
+(defun byte-compile-info-message (&rest args)
+  "Message format ARGS in a way that looks pleasing in the compilation output."
+  (message "%s" (concat "  INFO     " (apply #'format args))))
+
 \f
 ;; I nuked this because it's not a good idea for users to think of using it.
 ;; These options are a matter of installation preference, and have nothing to


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>   INFO     Processed 200 completions from files

This is perhaps more understandable...

+            (byte-compile-info-message "Scanned %d files for completions"

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Stefan Monnier]

> There's one point I'd argue for more messaging than we have today.  Or
> at least different messaging:
>
>   GEN      loaddefs.el
> Not registering prefix "lo" from completion.  Affects:
> ("locate-completion-entry" "locate-completion-entry-retry"
> "locate-completion-db-error" "load-completions-from-file")
> Not registering prefix "*" from ielm.  Affects: ("*" "**" "***" "*1" "*2" "*3")
> (etc)
>
> When we come to this point in the build process, even on my fastest
> machine, it pauses for many seconds.  I'm not sure that the "not
> registering" thing is useful, but it'd be nice if it gave more feedback
> to let us know that it was working on something and not having choked...

This is the step that builds the loaddefs.el file (i.e. all the
autoloads).  It also happens to build the "prefix" table used to try and
automatically know where to find which definitions, but that's
a side-gig.  So the processing doesn't have anything to do with
"completions", really (in the above warnings "completion" refers to the
completion.el package).

So, yes, you can output some progress messages if you want (it only
takes a long time during bootstrap.  On rebuilds it's usually very
fast), but makes it says something about "scraped autoloads from
N files" or something like that.

> I don't see much value in the "Not registering" messages, because I
> don't see how any developer would respond to the output, but I may be
> totally wrong here.

Those "not registering" mean that the corresponding definitions will not
be findable via things like `C-h o` until the package is loaded.  As for
what the developer should do: cleanup his namespace!


        Stefan

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> This is the step that builds the loaddefs.el file (i.e. all the
> autoloads).  It also happens to build the "prefix" table used to try and
> automatically know where to find which definitions, but that's
> a side-gig.  So the processing doesn't have anything to do with
> "completions", really (in the above warnings "completion" refers to the
> completion.el package).

Ah, I see.

> So, yes, you can output some progress messages if you want (it only
> takes a long time during bootstrap.  On rebuilds it's usually very
> fast), but makes it says something about "scraped autoloads from
> N files" or something like that.

OK; I'll change the patch to that.

>> I don't see much value in the "Not registering" messages, because I
>> don't see how any developer would respond to the output, but I may be
>> totally wrong here.
>
> Those "not registering" mean that the corresponding definitions will not
> be findable via things like `C-h o` until the package is loaded.  As for
> what the developer should do: cleanup his namespace!

:-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Stefan Monnier]

>> Those "not registering" mean that the corresponding definitions will not
>> be findable via things like `C-h o` until the package is loaded.  As for
>> what the developer should do: cleanup his namespace!
>
> :-)

Oh, there's no happy face about those messes, no!


        Stefan

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

I think we've covered almost everything now.  Here are the last two
things I'm seeing that I think are sub-optimal:

  GEN      ../lisp/leim/quail/tsang-b5.el
Converting /home/larsi/src/emacs/trunk/leim/CXTERM-DIC/PY-b5.tit to quail-package...
Decoding with coding system cn-big5...
Processing header part...
Formatting translation rules...

There's a long spew of these for all(?) files in quail.  I think the
four lines after the GEN aren't interesting and should be removed.

---

The "Scanning ... for" messages:

Scanning ./net for finder
  ELC      emacs-lisp/eieio.elc
  ELC      emacs-lisp/eieio-base.elc
  ELC      cedet/semantic/db.elc
  ELC      align.elc
  ELC      allout.elc
  ELC      allout-widgets.elc
Scanning ./language for custom
Scanning ./nxml for finder
Scanning ./org for finder
Scanning ./leim for custom
Scanning ./leim/ja-dic for custom
  ELC      ansi-color.elc
Scanning ./leim/quail for custom
Scanning ./play for finder
Scanning ./progmodes for finder

(This is with a make -j8 so they're intertwined with ELC files.)

I have no opinion on whether these are helpful messages or not, but I
think (if we want to keep them) that they should use the same format as
the new messages during autoload scraping, so change them to:

  INFO     Scanning ./language for custom

etc.  

---

And that's it!  We're done!  Tomorrow I can go back to reading the book
I've been stuck at on the same page for a week...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Clément Pit-Claudel]

On 2019-06-17 19:08, Stefan Monnier wrote:
>>> Those "not registering" mean that the corresponding definitions will not
>>> be findable via things like `C-h o` until the package is loaded.  As for
>>> what the developer should do: cleanup his namespace!
>>
>> :-)
> 
> Oh, there's no happy face about those messes, no!

Wait wait, some of these messages seemed wrong, too:

> Not registering prefix "is" from ind-util.  Affects: ("is13194-default-repertory" "is13194-repertory-to-ucs-script" "is13194-to-ucs-devanagari-hashtbl" "is13194-to-ucs-devanagari-regexp" "is13194-to-ucs-bengali-hashtbl" "is13194-to-ucs-bengali-regexp" "is13194-to-ucs-assamese-hashtbl" "is13194-to-ucs-assamese-regexp" "is13194-to-ucs-gurmukhi-hashtbl" "is13194-to-ucs-gurmukhi-regexp" "is13194-to-ucs-gujarati-hashtbl" "is13194-to-ucs-gujarati-regexp" "is13194-to-ucs-oriya-hashtbl" "is13194-to-ucs-oriya-regexp" "is13194-to-ucs-tamil-hashtbl" "is13194-to-ucs-tamil-regexp" "is13194-to-ucs-telugu-hashtbl" "is13194-to-ucs-telugu-regexp" "is13194-to-ucs-malayalam-hashtbl" "is13194-to-ucs-malayalam-regexp" "is13194-to-ucs-kannada-hashtbl" "is13194-to-ucs-kannada-regexp" "iscii-to-ucs-region")

Shouldn't the prefix be `is13194', instead of `is' (at least except for that last entry)?

Re: Towards a cleaner build: other output

[Stefan Monnier]

>> Not registering prefix "is" from ind-util.  Affects:
>> ("is13194-default-repertory" "is13194-repertory-to-ucs-script"
>> "is13194-to-ucs-devanagari-hashtbl" "is13194-to-ucs-devanagari-regexp"
>> "is13194-to-ucs-bengali-hashtbl" "is13194-to-ucs-bengali-regexp"
>> "is13194-to-ucs-assamese-hashtbl" "is13194-to-ucs-assamese-regexp"
>> "is13194-to-ucs-gurmukhi-hashtbl" "is13194-to-ucs-gurmukhi-regexp"
>> "is13194-to-ucs-gujarati-hashtbl" "is13194-to-ucs-gujarati-regexp"
>> "is13194-to-ucs-oriya-hashtbl" "is13194-to-ucs-oriya-regexp"
>> "is13194-to-ucs-tamil-hashtbl" "is13194-to-ucs-tamil-regexp"
>> "is13194-to-ucs-telugu-hashtbl" "is13194-to-ucs-telugu-regexp"
>> "is13194-to-ucs-malayalam-hashtbl" "is13194-to-ucs-malayalam-regexp"
>> "is13194-to-ucs-kannada-hashtbl" "is13194-to-ucs-kannada-regexp"
>> "iscii-to-ucs-region")
>
> Shouldn't the prefix be `is13194', instead of `is' (at least except for that last entry)?

There are already other prefixes in ind-util, hence the problem.
Indeed, the automatic guess works suboptimally in this file, but the
right fix is still to fix the namespace.


        Stefan

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> There are already other prefixes in ind-util, hence the problem.
> Indeed, the automatic guess works suboptimally in this file, but the
> right fix is still to fix the namespace.

Is the right fix here to split ind-util.el into two files, and have the
new file have all the is13914 symbols?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Stefan Monnier]

>> There are already other prefixes in ind-util, hence the problem.
>> Indeed, the automatic guess works suboptimally in this file, but the
>> right fix is still to fix the namespace.
> Is the right fix here to split ind-util.el into two files, and have the
> new file have all the is13914 symbols?

I don't think that's needed.  I suspect that adding an "indian-" prefix
to the ucs-to-* and iscii-* definitions in ind-util.el should be enough
to fix the problem.


        Stefan

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I don't think that's needed.  I suspect that adding an "indian-" prefix
> to the ucs-to-* and iscii-* definitions in ind-util.el should be enough
> to fix the problem.

Yup; that made the warning go away.  Are the other ones as trivial?

Not registering prefix "lo" from completion.  Affects: ("locate-completion-entry" "locate-completion-entry-retry" "locate-completion-db-error" "load-completions-from-file")
Not registering prefix "*" from ielm.  Affects: ("*" "**" "***" "*1" "*2" "*3")
Not registering prefix "se" from page-ext.  Affects: ("search-pages" "set-page-delimiter")

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Stefan Monnier]

> Yup; that made the warning go away.  Are the other ones as trivial?

Usually it's pretty trivial, yes, but it may require renaming exported
functions, commands, and variables.

> Not registering prefix "lo" from completion.  Affects:
> ("locate-completion-entry" "locate-completion-entry-retry"
> "locate-completion-db-error" "load-completions-from-file")

IIRC completion.el is really badly behaved w.r.t namespace cleanliness.
If you can clean this up, that'd be great, but I think it'll be less trivial.

> Not registering prefix "*" from ielm.  Affects: ("*" "**" "***" "*1" "*2" "*3")

These aren't really definitions, so it's harmless if their "prefix" is
not registered.  I guess we can also avoid the warning by not using
`defvar` on those things.

> Not registering prefix "se" from page-ext.  Affects: ("search-pages" "set-page-delimiter")

The right fix there is to rename "everything" to use a "page-" prefix rather
than merely using "page" somewhere in the name.


        Stefan

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Not registering prefix "*" from ielm.  Affects: ("*" "**" "***" "*1"
>> "*2" "*3")
>
> These aren't really definitions, so it's harmless if their "prefix" is
> not registered.  I guess we can also avoid the warning by not using
> `defvar` on those things.

Right, it's these:

(defvar * nil
  "Most recent value evaluated in IELM.")

But ielm really does define (and use) these variables, and you can
describe-variable them:

----
* is a variable defined in ‘ielm.el’.
Its value is 3
Local in buffer *ielm*; global value is nil

  Probably introduced at or before Emacs version 1.1.

Documentation:
Most recent value evaluated in IELM.
----

Which seems nice...

Perhaps there should be a mechanism to tell the autoload scraper to
ignore certain variables that are known not to be prefix-ey?  It would
basically just apply to these ielm variables...

>> Not registering prefix "se" from page-ext.  Affects: ("search-pages"
>> "set-page-delimiter")
>
> The right fix there is to rename "everything" to use a "page-" prefix rather
> than merely using "page" somewhere in the name.

I'll do the renaming and mark the old names as obsolete.  I can't see
them used anywhere else in the tree, but perhaps there are out-of-tree
uses...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Stefan Monnier]

> Right, it's these:
>
> (defvar * nil
>   "Most recent value evaluated in IELM.")
>
> But ielm really does define (and use) these variables,

Right, but the intent is for them to be used from IELM, rather than from
anywhere else.  They "bleed" out of IELM because of the way IELM is implemented.

> and you can describe-variable them: [...] Which seems nice...

Indeed you can, but I'm not sure it's that important.
I think turning those (defvar * nil) to (defvar *) would be fine: the
global value is always nil anyway, AFAICT.

>> The right fix there is to rename "everything" to use a "page-" prefix rather
>> than merely using "page" somewhere in the name.
> I'll do the renaming and mark the old names as obsolete.  I can't see
> them used anywhere else in the tree, but perhaps there are out-of-tree
> uses...

I think these are all user-level commands, so the most likely place for
them to appear is in ~/.emacs


        Stefan

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Right, but the intent is for them to be used from IELM, rather than
> from anywhere else.  They "bleed" out of IELM because of the way IELM
> is implemented.

Yup.  

>> and you can describe-variable them: [...] Which seems nice...
>
> Indeed you can, but I'm not sure it's that important.
> I think turning those (defvar * nil) to (defvar *) would be fine: the
> global value is always nil anyway, AFAICT.

It is...  And I think probably nobody would ever type in ** without
knowing what it is, so having a doc string there isn't really that
vital?

So I'll just go ahead and devariabilise them.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Noam Postavsky]

On Thu, 20 Jun 2019 at 07:51, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> It is...  And I think probably nobody would ever type in ** without
> knowing what it is, so having a doc string there isn't really that
> vital?
>
> So I'll just go ahead and devariabilise them.

Looks like you could still have the docstring via put, e.g.:

(put '* 'variable-documentation "Most recent value evaluated in IELM.")

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> Looks like you could still have the docstring via put, e.g.:
>
> (put '* 'variable-documentation "Most recent value evaluated in IELM.")

Oh, nice.  I'll put that in.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 20 Jun 2019 14:09:39 +0200
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
>  Emacs developers <emacs-devel@gnu.org>
> 
> Noam Postavsky <npostavs@gmail.com> writes:
> 
> > Looks like you could still have the docstring via put, e.g.:
> >
> > (put '* 'variable-documentation "Most recent value evaluated in IELM.")
> 
> Oh, nice.  I'll put that in.

Please always leave comments if you do something unusual, to explain
why this was done.  Otherwise someone will come later and delete it
thinking it's some weird non-standard trick.

Thanks.

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Please always leave comments if you do something unusual, to explain
> why this was done.  Otherwise someone will come later and delete it
> thinking it's some weird non-standard trick.

Yup; I've done that.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Stefan Monnier]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Noam Postavsky <npostavs@gmail.com> writes:
>
>> Looks like you could still have the docstring via put, e.g.:
>>
>> (put '* 'variable-documentation "Most recent value evaluated in IELM.")
>
> Oh, nice.  I'll put that in.

In `master` you can even use `internal--define-uninitialized-variable`,
which does all that `defvar` and `defconst` do except for setting the
actual variable.  And autoload.el won't recognize it as a definition.


        Stefan

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> (put '* 'variable-documentation "Most recent value evaluated in IELM.")
>>
>> Oh, nice.  I'll put that in.
>
> In `master` you can even use `internal--define-uninitialized-variable`,
> which does all that `defvar` and `defconst` do except for setting the
> actual variable.  And autoload.el won't recognize it as a definition.

Hm...  But is using `internal--define-uninitialized-variable' perhaps
more obscure than using (defvar sym) (put sym)?  If that's something
that can be used "non-internally", perhaps it should have a different
name?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Stefan Monnier]

>>>> (put '* 'variable-documentation "Most recent value evaluated in IELM.")
>>> Oh, nice.  I'll put that in.
>> In `master` you can even use `internal--define-uninitialized-variable`,
>> which does all that `defvar` and `defconst` do except for setting the
>> actual variable.  And autoload.el won't recognize it as a definition.
>
> Hm...  But is using `internal--define-uninitialized-variable' perhaps
> more obscure than using (defvar sym) (put sym)?  If that's something
> that can be used "non-internally", perhaps it should have a different
> name?

It's a brand new function, introduced to fix several corner case
problems in `defcustom`.  Maybe it should be less "internal", indeed, but
I don't know yet.

FWIW, the differences with defvar+put that I can see are:
1- the docstring is purecopied if applicable (it's not applicable here).
2- the var is globally declared as dynamically-scoped (rather than only
   in the current file).
3- the variable is added to `load-history`.

I'm not sure whether (2) and (3) are advantages or disadvantages in the
present case.


        Stefan

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> FWIW, the differences with defvar+put that I can see are:
> 1- the docstring is purecopied if applicable (it's not applicable here).
> 2- the var is globally declared as dynamically-scoped (rather than only
>    in the current file).
> 3- the variable is added to `load-history`.
>
> I'm not sure whether (2) and (3) are advantages or disadvantages in the
> present case.

I think...  perhaps the latter two are disadvantages here.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Stefan Monnier]

> I think...  perhaps the latter two are disadvantages here.

Then it makes sense to keep the internal-sounding name, I guess ;-)


        Stefan

Re: Towards a cleaner build: done!

[Lars Ingebrigtsen]

There's now just one Warning: line left (which is being pondered upon),
and some returning warnings from eieio about obsolete functions which
are, er, complicated to fix properly, but I think we're basically done
here.  :-)  Thanks for all the help with the myriad of questions from me
over the last week or so.

I think it now would be nice to have the option to have the equivalent
of -Werr for the list code: Stop compilation if we get a warning.

Frequently when I'm working on a set of files, I'll run "make" in the
Lisp directory to see whether any compilation problems have popped up.
I'll fix a problem, hit `g' in the compilation buffer and see that it
disappears...  but then if there were more files with problems, that's
gone from the window, because Emacs won't recompile those.

Which leads to a lot of wasted time while I try to remember what files
those might have been, so I normally just run a rm `find . -name
'*.elc'` instead, because that's faster than trying to remember, even if
it takes several minutes to then compile again.

So: An "ERROR_ON_WARNING=1 make" flag or something?  Or do we already
have that somewhere?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: done!

[Noam Postavsky]

On Thu, 20 Jun 2019 at 08:49, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> So: An "ERROR_ON_WARNING=1 make" flag or something?  Or do we already
> have that somewhere?

I think

make "BYTE_COMPILE_EXTRA_FLAGS=--eval '(setq byte-compile-error-on-warn t)'"

should already work.

Re: Towards a cleaner build: done!

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> On Thu, 20 Jun 2019 at 08:49, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
>> So: An "ERROR_ON_WARNING=1 make" flag or something?  Or do we already
>> have that somewhere?
>
> I think
>
> make "BYTE_COMPILE_EXTRA_FLAGS=--eval '(setq byte-compile-error-on-warn t)'"
>
> should already work.

Oh, cool!

Should we make this into a Makefile thing for easier use by anybody who
wants this, though?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: done!

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 20 Jun 2019 14:24:17 +0200
> 
> There's now just one Warning: line left (which is being pondered upon),

I've finished pondering and pushed a change to remove that last
warning.

Thanks again for doing this.

Re: Towards a cleaner build: done!

[Arash Esbati]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> There's now just one Warning: line left (which is being pondered upon),
> and some returning warnings from eieio about obsolete functions which
> are, er, complicated to fix properly, but I think we're basically done
> here.  :-)  Thanks for all the help with the myriad of questions from me
> over the last week or so.

I get this warning when building Emacs on Windows:

    In end of data:
    gnus/nnmaildir.el:1795:1:Warning: the function `unix-sync' is not known to be
        defined.

A patch fixing it is attached.  Many thanks for fixing all those
warnings, Emacs builds much cleaner now!

Best, Arash


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Suppress-warning-about-unix-sync-in-nnmaildir.el.patch --]
[-- Type: text/x-patch, Size: 945 bytes --]

From cc275331b4943e03d0c1c0e5264ab09974df7308 Mon Sep 17 00:00:00 2001
From: Arash Esbati <arash@gnu.org>
Date: Tue, 25 Jun 2019 23:05:17 +0200
Subject: [PATCH] Suppress warning about unix-sync in nnmaildir.el

* nnmaildir.el (nnmaildir-request-replace-article): Check if the
function `unix-sync' is bound before running it.
---
 lisp/gnus/nnmaildir.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/gnus/nnmaildir.el b/lisp/gnus/nnmaildir.el
index ac125c905a..3becee3511 100644
--- a/lisp/gnus/nnmaildir.el
+++ b/lisp/gnus/nnmaildir.el
@@ -1396,7 +1396,8 @@ nnmaildir-request-replace-article
       (with-current-buffer buffer
 	(write-region (point-min) (point-max) tmpfile nil 'no-message nil
 		      'excl))
-      (unix-sync) ;; no fsync :(
+      (when (fboundp 'unix-sync)
+	(unix-sync)) ;; no fsync :(
       (rename-file tmpfile (concat (nnmaildir--cur dir) file suffix) 'replace)
       t)))
 
-- 
2.22.0

Re: Towards a cleaner build: done!

[Lars Ingebrigtsen]

Arash Esbati <arash@gnu.org> writes:

> I get this warning when building Emacs on Windows:
>
>     In end of data:
>     gnus/nnmaildir.el:1795:1:Warning: the function `unix-sync' is not known to be
>         defined.
>
> A patch fixing it is attached. 

Thanks; I've applied it to the Emacs trunk.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Eli Zaretskii]

> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Tue, 18 Jun 2019 00:04:04 -0400
> 
> > Not registering prefix "is" from ind-util.  Affects: ("is13194-default-repertory" "is13194-repertory-to-ucs-script" "is13194-to-ucs-devanagari-hashtbl" "is13194-to-ucs-devanagari-regexp" "is13194-to-ucs-bengali-hashtbl" "is13194-to-ucs-bengali-regexp" "is13194-to-ucs-assamese-hashtbl" "is13194-to-ucs-assamese-regexp" "is13194-to-ucs-gurmukhi-hashtbl" "is13194-to-ucs-gurmukhi-regexp" "is13194-to-ucs-gujarati-hashtbl" "is13194-to-ucs-gujarati-regexp" "is13194-to-ucs-oriya-hashtbl" "is13194-to-ucs-oriya-regexp" "is13194-to-ucs-tamil-hashtbl" "is13194-to-ucs-tamil-regexp" "is13194-to-ucs-telugu-hashtbl" "is13194-to-ucs-telugu-regexp" "is13194-to-ucs-malayalam-hashtbl" "is13194-to-ucs-malayalam-regexp" "is13194-to-ucs-kannada-hashtbl" "is13194-to-ucs-kannada-regexp" "iscii-to-ucs-region")
> 
> Shouldn't the prefix be `is13194', instead of `is' (at least except for that last entry)?

The last entry _is_ the reason for trying to use "is".

Re: Towards a cleaner build: other output

[Stefan Monnier]

> eieio.el: ‘object-print’ is an obsolete generic function (as of 26.1); use
> ‘cl-print-object’ instead.
>
> These are output by cl-defmethod/cl-defgeneric, and we're defining an
> (obsolete) fallback method, so we should suppress this warning.  Do we
> need a new mechanism or do we have something to cover this?

Presumably wrapping those `cl-defmethod`s in a with-suppressed-warnings
should do the trick.

> *Warning* default value of <<no-type>> tokens changed to QUERY, was NULL_TOKEN
> *Warning* default value of <<no-type>> tokens changed to TRUE, was QUERY
> *Warning* default value of <<no-type>> tokens changed to FALSE, was TRUE
> Grammar in javascript.el contains 2 useless nonterminals and 4 useless rules
> Grammar in js-wy.el contains 2 useless nonterminals and 4 useless rules
> Grammar in python-wy.el contains 2 useless nonterminals and 3 useless rules
> Grammar in python-wy.el contains 4 shift/reduce conflicts
> Grammar in python.el contains 2 useless nonterminals and 3 useless rules
> Grammar in python.el contains 4 shift/reduce conflicts
>
> These are from cedet, and are warning about grammars containing...
> useless rules?  Perhaps this is more useful at run-time than at compile
> time?

This is like running `yacc`: you want those warnings when you run
`yacc`, not when you use the resulting grammar.

> Loading /home/larsi/src/emacs/trunk/lisp/international/uni-name.el (source)...
>
> Hm...  a "Loading" in the middle here?  Does anybody know why?

I guess something's using `load` instead of `require`?

> latin-ltx: 1535 rules (+ 0 conflicts)!
> Uhm...  That doesn't seem very useful to me -- only output that if there
> are any conflicts?

Agreed (and I plead guilty),


        Stefan

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> *Warning* default value of <<no-type>> tokens changed to QUERY, was NULL_TOKEN
>> *Warning* default value of <<no-type>> tokens changed to TRUE, was QUERY
>> *Warning* default value of <<no-type>> tokens changed to FALSE, was TRUE
>> Grammar in javascript.el contains 2 useless nonterminals and 4 useless rules
>> Grammar in js-wy.el contains 2 useless nonterminals and 4 useless rules
>> Grammar in python-wy.el contains 2 useless nonterminals and 3 useless rules
>> Grammar in python-wy.el contains 4 shift/reduce conflicts
>> Grammar in python.el contains 2 useless nonterminals and 3 useless rules
>> Grammar in python.el contains 4 shift/reduce conflicts
>>
>> These are from cedet, and are warning about grammars containing...
>> useless rules?  Perhaps this is more useful at run-time than at compile
>> time?
>
> This is like running `yacc`: you want those warnings when you run
> `yacc`, not when you use the resulting grammar.

I thought it was the .el files themselves that were complaining, but
poking at it a bit more, it's a warning when generating the .el files
from .wy files, so the warnings seem correct, indeed.

>> Loading /home/larsi/src/emacs/trunk/lisp/international/uni-name.el (source)...
>>
>> Hm...  a "Loading" in the middle here?  Does anybody know why?
>
> I guess something's using `load` instead of `require`?

Yup.  It's this code:

  (defmacro latin-ltx--define-rules (&rest rules)
    (load "uni-name")

There's no comment about why, but it's pretty obvious when looking at
the file itself -- it has no `provide'.  Is that an oversight or is it
meant to be that way?  These files are all autogenerated...

;; Copyright © 1991-2019 Unicode, Inc.
;; Generated from Unicode data files by unidata-gen.el.
;; The sources for this file are found in the admin/unidata/ directory in
;; the Emacs sources.  The Unicode data files are used under the
;; Unicode Terms of Use, as contained in the file copyright.html in that
;; same directory.

Perhaps just add a NOMESSAGE parameter to that load?  

>> latin-ltx: 1535 rules (+ 0 conflicts)!
>> Uhm...  That doesn't seem very useful to me -- only output that if there
>> are any conflicts?
>
> Agreed (and I plead guilty),

OK; I'll silence that message (when there are zero conflicts).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> *Warning* default value of <<no-type>> tokens changed to QUERY, was NULL_TOKEN
>> *Warning* default value of <<no-type>> tokens changed to TRUE, was QUERY
>> *Warning* default value of <<no-type>> tokens changed to FALSE, was TRUE
>> Grammar in javascript.el contains 2 useless nonterminals and 4 useless rules
>> Grammar in js-wy.el contains 2 useless nonterminals and 4 useless rules
>> Grammar in python-wy.el contains 2 useless nonterminals and 3 useless rules
>> Grammar in python-wy.el contains 4 shift/reduce conflicts
>> Grammar in python.el contains 2 useless nonterminals and 3 useless rules
>> Grammar in python.el contains 4 shift/reduce conflicts
>>
>> These are from cedet, and are warning about grammars containing...
>> useless rules?  Perhaps this is more useful at run-time than at compile
>> time?
>
> This is like running `yacc`: you want those warnings when you run
> `yacc`, not when you use the resulting grammar.

I switched debugging on in wisent and made it tell me what nonterminals
were useless and fixed up the .wy grammars, so now there's no warnings
about that any more.

This is the only remaining output from parsing the grammars now:

Grammar in python-wy.el contains 4 shift/reduce conflicts

Which I have absolutely no idea how to solve...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Stefan Monnier]

> This is the only remaining output from parsing the grammars now:
> Grammar in python-wy.el contains 4 shift/reduce conflicts
> Which I have absolutely no idea how to solve...

Very often those shift-reduce conflicts are harmless (shift is indeed
the right choice) and fixing them involves undesirable changes to the
grammar (changing its structure in "unnatural" ways, making it harder
to read&understand).

So we should either learn to live with this, or provide some way to tell
wisent "expect 4 shift-reduce conflicts".


        Stefan

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> So we should either learn to live with this, or provide some way to tell
> wisent "expect 4 shift-reduce conflicts".

Hm...  I think I'll take a stab at adding a method to tell wisent to
expect those shift/reduce conflicts...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> So we should either learn to live with this, or provide some way to tell
>> wisent "expect 4 shift-reduce conflicts".
>
> Hm...  I think I'll take a stab at adding a method to tell wisent to
> expect those shift/reduce conflicts...

That was easy.  I added a new spec to the .wy files that allowed
per-grammaer specification of the shift/reduce conflicts.

But after I did that I noticed this in comp.el:

(defcustom wisent-expected-conflicts nil
  "If non-nil suppress the warning about shift/reduce conflicts.
It is a decimal integer N that says there should be no warning if
there are N shift/reduce conflicts and no reduce/reduce conflicts.  A
warning is given if there are either more or fewer conflicts, or if
there are any reduce/reduce conflicts."
  :group 'wisent
  :type '(choice (const nil) integer))

So somebody has struggled with this before, apparently.  :-/  But since
that variable is not a per-grammar setting, it doesn't seem very
useful..

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: other output

[Stefan Monnier]

> So somebody has struggled with this before, apparently.  :-/  But since
> that variable is not a per-grammar setting, it doesn't seem very
> useful..

So you can mark this variable as obsolete after you add your own take
on this.


        Stefan

Re: Towards a cleaner build: other output

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> So you can mark this variable as obsolete after you add your own take
> on this.

Will do.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: viper-ex

[Lars Ingebrigtsen]

In ex-tag:
emulation/viper-ex.el:2019:16:Warning: `find-tag' is an obsolete function (as
    of 25.1); use `xref-find-definitions' instead.
emulation/viper-ex.el:2020:14:Warning: `find-tag-other-window' is an obsolete
    function (as of 25.1); use `xref-find-definitions-other-window' instead.

I thought this was going to be trivial to fix, but:

	  (if (string= tag "")
	      (find-tag ex-tag t)
	    (find-tag-other-window ex-tag))

find-tag:

If second arg NEXT-P is t (interactively, with prefix arg), search for
another tag that matches the last tagname or regexp used.

--

But xref-find-definitions doesn't have a next-p argument...  

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: project.el

[Lars Ingebrigtsen]

In end of data:
progmodes/project.el:570:1:Warning: the following functions are not known to
    be defined: xref--regexp-to-extended, xref--convert-hits

project.el is calling internal xref functions -- should those be changed
to be external?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: obsolete/cl-compat.el

[Lars Ingebrigtsen]

In zip-lists:
obsolete/cl-compat.el:145:4:Warning: mapcan called with 3 arguments, but
    accepts only 2

This file has been obsolete since Emacs 23.3 -- is it OK to remove it?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: erc.el

[Lars Ingebrigtsen]

In erc-send-input:
erc/erc.el:5462:9:Warning: global/dynamic var `str' lacks a prefix

The problem here is that `str' is in the public calling converion on
`erc-pre-send-hook': 

---
(defcustom erc-send-pre-hook nil
  "Hook called first when some text is sent through `erc-send-current-line'.
It gets called with one argument, STRING.

To change the text that will be sent, set the variable `str' which is
used in `erc-send-current-line'.
---

That's a quite, er, uhm, strange way of doing it, but...  The
traditional way is to offer a filter function that can alter the string.

Would a way forward here be to obsolete this hook variable and add a new
function along those lines?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: calendar

[Lars Ingebrigtsen]

22% of the remaining byte-compilation warnings (if we ignore CEDET; I
swoon every time I have a peek at it) are from calendar:

    78:calendar/cal-bahai.el:316:1:Warning: global/dynamic var `date' lacks a prefix
     82:calendar/cal-china.el:635:1:Warning: global/dynamic var `date' lacks a prefix
     83:calendar/cal-china.el:653:1:Warning: global/dynamic var `entry' lacks a prefix
     86:calendar/cal-coptic.el:171:1:Warning: global/dynamic var `date' lacks a prefix
     91:calendar/cal-french.el:246:1:Warning: global/dynamic var `date' lacks a prefix
     97:calendar/cal-hebrew.el:751:1:Warning: global/dynamic var `date' lacks a prefix
     98:calendar/cal-hebrew.el:759:1:Warning: global/dynamic var `entry' lacks a
    103:calendar/cal-islam.el:308:1:Warning: global/dynamic var `date' lacks a prefix
    107:calendar/cal-iso.el:132:1:Warning: global/dynamic var `date' lacks a prefix
    111:calendar/cal-julian.el:186:1:Warning: global/dynamic var `date' lacks a prefix
    115:calendar/cal-mayan.el:356:1:Warning: global/dynamic var `date' lacks a prefix
    121:calendar/cal-persia.el:199:1:Warning: global/dynamic var `date' lacks a prefix
    129:calendar/lunar.el:244:1:Warning: global/dynamic var `date' lacks a prefix

And they're apparently used to pass in values to specific functions; not
user-defined ones which are usually the problem with these things:

;; To be called from diary-list-sexp-entries, where DATE is bound.
;;;###diary-autoload
(defun diary-bahai-date ()
  "Bahá’í calendar equivalent of date diary entry."
  (format "Bahá’í date: %s" (calendar-bahai-date-string date)))

;; To be called from diary-list-sexp-entries, where DATE is bound.
;;;###diary-autoload
(defun diary-french-date ()
  "French calendar equivalent of date diary entry."
  (let ((f (calendar-french-date-string date)))
    (if (string-equal f "")
        "Date is pre-French Revolution"
      (format "French Revolutionary date: %s" f))))

So...  I'm probably missing something here, because can't we just change
the calling convention of these functions to take a `date' parameter?

I don't use the calendar myself, though.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> 22% of the remaining byte-compilation warnings (if we ignore CEDET; I
> swoon every time I have a peek at it) are from calendar:

But if we don't ignore cedet...  what's up with all those warnings
anyway?

To take one of the 150 or so examples:

cedet/ede/config.el:388:5:Warning: Unknown slot `:c-preprocessor-table'

That's from:

    (setq filemap (append filemap (oref config :c-preprocessor-table)))

which results in this compiler macro from eieio-oref:

              (pcase slot
                ((and (or `',name (and name (pred keywordp)))
                      (guard (not (memq name eieio--known-slot-names))))
                 (macroexp--warn-and-return
                  (format-message "Unknown slot `%S'" name) exp 'compile-only))

So...  It's saying that the slot name is not in that internal variable?
But it's populated by that defclass earlier in the file?

Yes:

eieio--known-slot-names =>
(c-preprocessor-files c-preprocessor-table c-include-path tag-info length location summary message position buffer column line file search-function create-function data port user host source type)

Oh!  Those are without colons?  Is that the entire issue here, that the
check is strict about colons/not colons and the rest of cedet doesn't
care?

(I've never used any part of cedet.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

There's a bunch of unit tests for cedet at
test/manual/cedet/cedet-utests.el, but there's no explanation on how to
run it.

There's

(defun cedet-utest (&optional exit-on-error)

[...]

  (if (or (not (featurep 'semanticdb-mode))
	  (not (semanticdb-minor-mode-p)))
      (error "CEDET Tests require: M-x semantic-load-enable-minimum-features"))

at the start, which seems nonsensical, because there's apparently no
such feature as semanticdb-mode (according to grep), and there's no
`semantic-load-enable-minimum-features' either...

That's apparently present in the cedet out-of-tree distribution, but
it's unclear whether any of this has ever worked...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Noam Postavsky]

On Thu, 13 Jun 2019 at 11:34, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
> There's a bunch of unit tests for cedet at
> test/manual/cedet/cedet-utests.el, but there's no explanation on how to
> run it.

> That's apparently present in the cedet out-of-tree distribution, but
> it's unclear whether any of this has ever worked...

There's some mention of tests in the "CEDET merge bug". Not sure if
there's any prospect of that ever being resolved though.

https://debbugs.gnu.org/28878

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> There's some mention of tests in the "CEDET merge bug". Not sure if
> there's any prospect of that ever being resolved though.
>
> https://debbugs.gnu.org/28878

Oh, it's still being maintained out-of-tree?  I assumed that that was
not the case due to the number of in-tree changes to cedet over the past
few years...

In that case, perhaps these cleanup fixes aren't that welcome?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Noam Postavsky]

On Thu, 13 Jun 2019 at 11:56, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
> > There's some mention of tests in the "CEDET merge bug". Not sure if
> > there's any prospect of that ever being resolved though.
> >
> > https://debbugs.gnu.org/28878
>
> Oh, it's still being maintained out-of-tree?  I assumed that that was
> not the case due to the number of in-tree changes to cedet over the past
> few years...

Well, it's not being maintained out-of-tree,
https://sourceforge.net/p/cedet/git/ci/master/tree/ shows last commit
in November 2017.
But from what I understand of Bug#28878 and #23792, there are many
change that still have never been merged...

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> Well, it's not being maintained out-of-tree,
> https://sourceforge.net/p/cedet/git/ci/master/tree/ shows last commit
> in November 2017.
> But from what I understand of Bug#28878 and #23792, there are many
> change that still have never been merged...

I see.  That's an unfortunate situation...

I had a look at the patch in #23792, and while it's big, it didn't
really look that daunting...  Is the situation that none of the people
with Emacs commit rights are really using cedet themselves?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Oh!  Those are without colons?  Is that the entire issue here, that the
> check is strict about colons/not colons and the rest of cedet doesn't
> care?

Let's try!

(require 'semantic/analyze)
(setq context (make-instance 'semantic-analyze-context :buffer (current-buffer)))
=> #s(semantic-analyze-context unbound unbound unbound unbound unbound #<buffer *unsent wide reply to Lars Ingebrigtsen*> unbound)

(oref context :buffer)
=> #<buffer *unsent wide reply to Lars Ingebrigtsen*>

(oref context buffer)
=> #<buffer *unsent wide reply to Lars Ingebrigtsen*>

Surely it's can be that easy -- just remove the colons to the 111
accesses with coloned slot names to get rid of the 111 warnings?

Comments please.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> (oref context :buffer)
> => #<buffer *unsent wide reply to Lars Ingebrigtsen*>
>
> (oref context buffer)
> => #<buffer *unsent wide reply to Lars Ingebrigtsen*>

Oh!

             ;; Accessing a slot via its :initarg is accepted by EIEIO
             ;; (but not CLOS) but is a bad idea (for one: it's slower).

So it does seem like the right fix is just to go CLOS-style on all the
oref/osets instead of :eieio...

I tried it, and everything compiles (with 111 fewer warnings :-)), so
perhaps I should just push it?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Stefan Monnier]

> cedet/ede/config.el:388:5:Warning: Unknown slot `:c-preprocessor-table'
[...]
> Oh!  Those are without colons?  Is that the entire issue here, that the
> check is strict about colons/not colons and the rest of cedet doesn't
> care?

Here's the story:
- in CLOS, you can access slots with `slot-value` where you specify the
  slot-name (typically a non-keyword symbol).
- in CLOS, `defclass` can specify for each slot an "initializer"
  keyword, which is the keyword to pass to `make-instance` to provide an
  initial value for that slot.
  Typically, this initializer keyword is the slot name with a leading ":".
- when Eric wrote EIEIO, he made his version of `slot-value` accept
  the use of the initializer keyword as an alias for the slot-name.
  [ In theory, this indirection gives you a bit of extra flexibility
    since a given initializer keyword can be mapped to different
    slot-names in different classes, but I've never seen any code take
    advantage of that nor can I easily imagine a use-case where this
    would come in handy.  ]
- in CEDET a lot of the code uses the initializer keyword rather than
  the slot-name for some reason (I assume Eric started it and the rest
  followed).  AFAICT most people don't really understand the difference
  between initializer keyword and slot-name in EIEIO for that reason.
- Using the initializer keyword rather than the slot name is actually
  marginally slower.

I'd like to deprecate this "feature" of EIEIO which just makes things
(marginally) slower and makes EIEIO unnecessarily different from CLOS.

So the warning only checks if the slot-name provided to `slot-value` is
known, and doesn't try to accommodate initializer keywords since I think
that should be deprecated.

So for 99% of those warnings, the fix is to remove the leading `:` (or
to replace it with a quote, depending on whether it's passed to
a function like `slot-value` or to a macro like `oref`).


        Stefan

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> So for 99% of those warnings, the fix is to remove the leading `:` (or
> to replace it with a quote, depending on whether it's passed to
> a function like `slot-value` or to a macro like `oref`).

Thanks for the explanation and the history behind this...

I'll go through all the warnings emitted and adjust the
oref/oset/slot-value calls and push after I've figured out how to
actually test this stuff.  I mean, it looks like it should be a
semi-mechanical replacement situation, but I hate not testing these
things...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Stefan Monnier]

> I'll go through all the warnings emitted and adjust the
> oref/oset/slot-value calls and push after I've figured out how to
> actually test this stuff.  I mean, it looks like it should be a
> semi-mechanical replacement situation, but I hate not testing these
> things...

The non-mechanical part is to look for the :initarg to see if
indeed :<foo> is the init arg for the <foo> slot.
This is *usually* the case, but it's just a coding convention,


        Stefan

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I'll go through all the warnings emitted and adjust the
>> oref/oset/slot-value calls and push after I've figured out how to
>> actually test this stuff.  I mean, it looks like it should be a
>> semi-mechanical replacement situation, but I hate not testing these
>> things...
>
> The non-mechanical part is to look for the :initarg to see if
> indeed :<foo> is the init arg for the <foo> slot.
> This is *usually* the case, but it's just a coding convention,

I grepped through :initarg in cedet, and it looks like there's only one:

   (max-tags-initial :initarg max-tags-initial

And my changes didn't change any of the call sites...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Stefan Monnier]

>>> I'll go through all the warnings emitted and adjust the
>>> oref/oset/slot-value calls and push after I've figured out how to
>>> actually test this stuff.  I mean, it looks like it should be a
>>> semi-mechanical replacement situation, but I hate not testing these
>>> things...
>>
>> The non-mechanical part is to look for the :initarg to see if
>> indeed :<foo> is the init arg for the <foo> slot.
>> This is *usually* the case, but it's just a coding convention,
>
> I grepped through :initarg in cedet, and it looks like there's only one:
>
>    (max-tags-initial :initarg max-tags-initial
>
> And my changes didn't change any of the call sites...

Then I guess you can just remove the `:`s and be done with it.


        Stefan

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Then I guess you can just remove the `:`s and be done with it.

I have now done so.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

OK, I've now sort-of tested cedet with this:

(defun lars-cedet ()
  (setq semantic-default-submodes
	'(global-semantic-highlight-func-mode
	  global-semantic-decoration-mode
	  global-semantic-stickyfunc-mode
	  global-semantic-idle-completions-mode
	  global-semantic-idle-scheduler-mode
	  global-semanticdb-minor-mode
	  global-semantic-idle-summary-mode
	  global-semantic-mru-bookmark-mode
	  global-cedet-m3-minor-mode
	  global-semantic-idle-local-symbol-highlight-mode
	  global-semantic-highlight-edits-mode
	  global-semantic-show-unmatched-syntax-mode
	  global-semantic-show-parser-state-mode))
  (semantic-mode 1))

I opened a .c file and saw that cedet did a lot of stuff, and then I
changed all the coloned oref/osets, built again, and checked again, and
things looked the same to me.

That's a kind of weak test, but at least it didn't blow up completely,
so at least I verified that...

I'll push the mega-patch.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

Now that most of the warnings are gone in cedet, perhaps one of the
remaining ones is actually a bug:

In toplevel form:
cedet/ede/config.el:409:36:Warning: Unknown slot `classpath'

It's this code:

(cl-defmethod ede-java-classpath ((proj ede-project-with-config-java))
  "Return the classpath for this project."
  (oref (ede-config-get-configuration proj) classpath))

But...  no classes define a classpath slot:

-*- mode: grep; default-directory: "~/src/emacs/trunk/lisp/cedet/" -*-
Grep started at Fri Jun 14 15:18:50

grep --color -nH --null -e classpath `find . -name '*.el'`
./ede/config.el:(cl-defmethod ede-java-classpath ((proj ede-project-with-config-java))
./ede/config.el:  "Return the classpath for this project."
./ede/config.el:  (oref (ede-config-get-configuration proj) classpath))
./ede.el:(cl-defmethod ede-java-classpath ((_this ede-project))
./ede.el:  "Return the classpath for this project."

Seems kinda suspicious?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Stefan Monnier]

> Now that most of the warnings are gone in cedet, perhaps one of the
> remaining ones is actually a bug:
>
> In toplevel form:
> cedet/ede/config.el:409:36:Warning: Unknown slot `classpath'
[...]
> Seems kinda suspicious?

Indeed, maybe ask Eric?


        Stefan

Re: Towards a cleaner build: cedet

[Dmitry Gutov]

On 14.06.2019 16:22, Lars Ingebrigtsen wrote:
> Now that most of the warnings are gone in cedet, perhaps one of the
> remaining ones is actually a bug:
> 
> In toplevel form:
> cedet/ede/config.el:409:36:Warning: Unknown slot `classpath'
> 
> It's this code:
> 
> (cl-defmethod ede-java-classpath ((proj ede-project-with-config-java))
>    "Return the classpath for this project."
>    (oref (ede-config-get-configuration proj) classpath))
> 
> But...  no classes define a classpath slot:

IIRC, Java support was never merged into Emacs because of copyright 
assignment problems (it had a different author, I guess).

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> (cl-defmethod ede-java-classpath ((proj ede-project-with-config-java))
>>    "Return the classpath for this project."
>>    (oref (ede-config-get-configuration proj) classpath))
>>
>> But...  no classes define a classpath slot:
>
> IIRC, Java support was never merged into Emacs because of copyright
> assignment problems (it had a different author, I guess).

So this is glue code to bits of cedet that isn't in Emacs?  Hm...
Tricky...

Should the method just be removed, I wonder, and be part of the
non-merged part of cedet?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Dmitry Gutov]

On 15.06.2019 15:41, Lars Ingebrigtsen wrote:
> So this is glue code to bits of cedet that isn't in Emacs?  Hm...
> Tricky...
> 
> Should the method just be removed, I wonder, and be part of the
> non-merged part of cedet?

*shrug* I wouldn't do that, myself.

No real benefit to the user, and (slightly) more difficulty when merging 
changes there or back.

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 15.06.2019 15:41, Lars Ingebrigtsen wrote:
>> So this is glue code to bits of cedet that isn't in Emacs?  Hm...
>> Tricky...
>>
>> Should the method just be removed, I wonder, and be part of the
>> non-merged part of cedet?
>
> *shrug* I wouldn't do that, myself.
>
> No real benefit to the user, and (slightly) more difficulty when
> merging changes there or back.

Sure.  But the way Emacs usually work with external packages is that
they provide the glue code, isn't it?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: cedet

[Dmitry Gutov]

On 15.06.2019 19:05, Lars Ingebrigtsen wrote:
> Sure.  But the way Emacs usually work with external packages is that
> they provide the glue code, isn't it?

That doesn't negate my point. CEDET is largely unmaintained now, I think.

If you want to take on the "other" side as well, sure. Otherwise, I'd 
just wait until we collectively throw our hands up and remove it. 
(Leaving in just the few bits other core packages are using).

Re: Towards a cleaner build: cedet

[Eric Ludlam]

On 6/13/19 12:35 PM, Stefan Monnier wrote:
>> cedet/ede/config.el:388:5:Warning: Unknown slot `:c-preprocessor-table'
> [...]
>> Oh!  Those are without colons?  Is that the entire issue here, that the
>> check is strict about colons/not colons and the rest of cedet doesn't
>> care?
> 
> Here's the story:
> - in CLOS, you can access slots with `slot-value` where you specify the
>    slot-name (typically a non-keyword symbol).

[...]

> I'd like to deprecate this "feature" of EIEIO which just makes things
> (marginally) slower and makes EIEIO unnecessarily different from CLOS.
> 


Thanks for looking into this.  I was not familiar with CLOS when I 
implemented EIEIO, as I was mainly focused on creating something I could 
use to build CEDET.  Anything that makes the system more compliant with 
standards sounds like a good idea to me.

To poke at other topics from this thread; there is an externally 
maintained CEDET, but it has been dormant for a while.  The external 
repository is b/c I couldn't get a general release from my company which 
would allow me to work in Emacs proper, so I needed to batch changes up 
and release them together.  Sadly, I haven't been developing code for 
several years, and never taught myself enough git to be able to do the 
merges.  As a bonus those merges were too much of a pain for others once 
EIEIO in Emacs changed, so the project just fizzled.

There are a small number of fixes in CEDET on SourceForge not in Emacs, 
but nothing too significant.  Mostly around some EDE & arduino / android 
stuff I had been fiddling with at home.

There was a question about tests for CEDET.  In the Sourceforge CEDET 
repository you can run the tests with Make, but those weren't merged 
into Emacs.  There are two collections, one set are unit tests.  Those 
are independent and could be merged a piece at a time.  Another set was 
more of an integration test.  Those might be tricky to bring over since 
not all of CEDET was merged into Emacs.  They also depend on compilers 
and such being installed.

Hope this helps
Eric

Re: Towards a cleaner build: cedet

[Lars Ingebrigtsen]

Eric Ludlam <eric@siege-engine.com> writes:

> There was a question about tests for CEDET.  In the Sourceforge CEDET
> repository you can run the tests with Make, but those weren't merged
> into Emacs.  There are two collections, one set are unit tests.  Those
> are independent and could be merged a piece at a time.  Another set
> was more of an integration test.  Those might be tricky to bring over
> since not all of CEDET was merged into Emacs.  They also depend on
> compilers and such being installed.

Having (at least) the unit tests merged into Emacs would be really
helpful, I think.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: calendar

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>     78:calendar/cal-bahai.el:316:1:Warning: global/dynamic var `date' lacks a prefix

(etc)

> So...  I'm probably missing something here, because can't we just change
> the calling convention of these functions to take a `date' parameter?

I was indeed missing something: These functions are apparently called
from sexp forms in the calendar files, so it's something that presumably
have to be supported forever (because we don't want to break people's
calendar files).

So is the right thing to do here just to suppress these warnings?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: calendar

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 14 Jun 2019 16:30:52 +0200
> 
> I was indeed missing something: These functions are apparently called
> from sexp forms in the calendar files, so it's something that presumably
> have to be supported forever (because we don't want to break people's
> calendar files).
> 
> So is the right thing to do here just to suppress these warnings?

Yes, I think so.  Preferably with a comment explaining why.

Thanks.

Re: Towards a cleaner build: calendar

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> So is the right thing to do here just to suppress these warnings?
>
> Yes, I think so.  Preferably with a comment explaining why.

Great; I've now done so.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: disass.el

[Lars Ingebrigtsen]

In disassemble-1:
emacs-lisp/disass.el:171:42:Warning: `string-as-unibyte' is an obsolete
    function (as of 26.1); use `encode-coding-string'.

It's this code:

    (if (consp obj)
	(setq bytes (car (cdr obj))		;the byte code
	      constvec (car (cdr (cdr obj))))	;constant vector
      ;; If it is lazy-loaded, load it now
      (fetch-bytecode obj)
      (setq bytes (aref obj 1)
	    constvec (aref obj 2)))
    (let ((lap (byte-decompile-bytecode (string-as-unibyte bytes) constvec))


But...

(multibyte-string-p (aref (byte-compile '(lambda (foo) (+ 4 foo))) 1))
=> nil

Is there any way the byte string in a byte-compiled function is
multibyte?  It kinda looks like a NOP to me...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: disass.el

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 13 Jun 2019 02:27:32 +0200
> 
>     (if (consp obj)
> 	(setq bytes (car (cdr obj))		;the byte code
> 	      constvec (car (cdr (cdr obj))))	;constant vector
>       ;; If it is lazy-loaded, load it now
>       (fetch-bytecode obj)
>       (setq bytes (aref obj 1)
> 	    constvec (aref obj 2)))
>     (let ((lap (byte-decompile-bytecode (string-as-unibyte bytes) constvec))
> 
> 
> But...
> 
> (multibyte-string-p (aref (byte-compile '(lambda (foo) (+ 4 foo))) 1))
> => nil
> 
> Is there any way the byte string in a byte-compiled function is
> multibyte?  It kinda looks like a NOP to me...

I suggest to remove the call and add an assertion there.  This way, we
will soon know if its a NOP or not.

Thanks.

Re: Towards a cleaner build: disass.el

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I suggest to remove the call and add an assertion there.  This way, we
> will soon know if its a NOP or not.

OK; I've now done that, and I tried disassembling a couple of functions,
and didn't hit the assertion.  

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: disass.el

[Stefan Monnier]

> (multibyte-string-p (aref (byte-compile '(lambda (foo) (+ 4 foo))) 1))
> => nil

IIRC the byte compiler emitted multibyte bytecode strings (at least
sometimes) in the distant past, so that's probably where it came from.
It should all be unibyte nowadays, so we can drop this now, indeed.


        Stefan

Re: Towards a cleaner build: erc.el

[Stefan Monnier]

> The problem here is that `str' is in the public calling convention on
> `erc-pre-send-hook': 
[...]
> That's a quite, er, uhm, strange way of doing it, but...  The
> traditional way is to offer a filter function that can alter the string.

I typically use (with-no-warnings (defvar str)) within the function
(but before the run-hook) for that.

> Would a way forward here be to obsolete this hook variable and add a new
> function along those lines?

I haven't looked in detail, but an erc-pre-send-function that filters the
string sounds good, yes.


        Stefan

Re: Towards a cleaner build: erc.el

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Would a way forward here be to obsolete this hook variable and add a new
>> function along those lines?
>
> I haven't looked in detail, but an erc-pre-send-function that filters the
> string sounds good, yes.

OK, I'll add that and deprecate the hook and adjust the documentation.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: project.el

[Dmitry Gutov]

On 13.06.2019 2:35, Lars Ingebrigtsen wrote:
> In end of data:
> progmodes/project.el:570:1:Warning: the following functions are not known to
>      be defined: xref--regexp-to-extended, xref--convert-hits
> 
> project.el is calling internal xref functions -- should those be changed
> to be external?

I'd rather not (at least, not yet).

I'm not very confident that these are the functions are want in the API 
for the third-party code to use, and for us to support in the coming 
years. I'm hoping to take care of this before the Emacs 27 release, however.

regexp-to-extended, by the way, could use a separate package.

Re: Towards a cleaner build: project.el

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 13.06.2019 2:35, Lars Ingebrigtsen wrote:
>> In end of data:
>> progmodes/project.el:570:1:Warning: the following functions are not known to
>>      be defined: xref--regexp-to-extended, xref--convert-hits
>>
>> project.el is calling internal xref functions -- should those be changed
>> to be external?
>
> I'd rather not (at least, not yet).

OK, then I think the right solution here is to just make project.el
require xref, and if you later change the functions, then we'll get a
new error at that point.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: project.el

[Dmitry Gutov]

On 13.06.2019 15:58, Lars Ingebrigtsen wrote:

> OK, then I think the right solution here is to just make project.el
> require xref, and if you later change the functions, then we'll get a
> new error at that point.

Fine by me, but we've previously dealt with these warnings using 
declare-function, see

(declare-function xref--show-xrefs "xref")
(declare-function xref-backend-identifier-at-point "xref")
(declare-function xref--find-ignores-arguments "xref")

Re: Towards a cleaner build: project.el

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> OK, then I think the right solution here is to just make project.el
>> require xref, and if you later change the functions, then we'll get a
>> new error at that point.
>
> Fine by me, but we've previously dealt with these warnings using
> declare-function, see
>
> (declare-function xref--show-xrefs "xref")
> (declare-function xref-backend-identifier-at-point "xref")
> (declare-function xref--find-ignores-arguments "xref")

That's a better idea, especially since xref requires project.el, so just
requiring xref would be circular.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: byte-run.el

[Lars Ingebrigtsen]

Here's an intractable one:

In toplevel form:
emacs-lisp/byte-run.el:48:9:Warning: `macro-declaration-function' is an
    obsolete function (as of 24.3); use `macro-declarations-alist' instead.

It's this code:

(defvar macro-declaration-function #'macro-declaration-function
  "Function to process declarations in a macro definition.
The function will be called with two args MACRO and DECL.
MACRO is the name of the macro being defined.
DECL is a list `(declare ...)' containing the declarations.
The value the function returns is not used.")

Both the variable and the function `macro-declaration-function' are
obsolete.

I can't suppress the warning, because at this point the macrology in
bytecomp.el isn't quite in place yet, and slapping a
`with-suppressed-warnings' either around the entire defvar, or changing
the definition to

(lambda (..)
  (with-suppressed-errror
    (macro-declaration-function ...)))

both fail.

Any ideas?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: byte-run.el

[Stefan Monnier]

> In toplevel form:
> emacs-lisp/byte-run.el:48:9:Warning: `macro-declaration-function' is an
>     obsolete function (as of 24.3); use `macro-declarations-alist' instead.
>
> It's this code:
>
> (defvar macro-declaration-function #'macro-declaration-function

Removing the `#` might be a good workaround.


        Stefan

Re: Towards a cleaner build: byte-run.el

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> It's this code:
>>
>> (defvar macro-declaration-function #'macro-declaration-function
>
> Removing the `#` might be a good workaround.

Oh, duh.  I've now done that (and ran a make bootstrap) to ensure that
this doesn't lead to any weird build problems, but things seem fine to
me, so I'm pushing it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: viper-ex

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 13 Jun 2019 01:21:36 +0200
> 
> In ex-tag:
> emulation/viper-ex.el:2019:16:Warning: `find-tag' is an obsolete function (as
>     of 25.1); use `xref-find-definitions' instead.
> emulation/viper-ex.el:2020:14:Warning: `find-tag-other-window' is an obsolete
>     function (as of 25.1); use `xref-find-definitions-other-window' instead.
> 
> I thought this was going to be trivial to fix, but:
> 
> 	  (if (string= tag "")
> 	      (find-tag ex-tag t)
> 	    (find-tag-other-window ex-tag))

Time to use the new anti-warning facilities you just introduced?

Re: Towards a cleaner build: viper-ex

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> In ex-tag:
>> emulation/viper-ex.el:2019:16:Warning: `find-tag' is an obsolete function (as
>>     of 25.1); use `xref-find-definitions' instead.
>> emulation/viper-ex.el:2020:14:Warning: `find-tag-other-window' is an obsolete
>>     function (as of 25.1); use `xref-find-definitions-other-window' instead.
>> 
>> I thought this was going to be trivial to fix, but:
>> 
>> 	  (if (string= tag "")
>> 	      (find-tag ex-tag t)
>> 	    (find-tag-other-window ex-tag))
>
> Time to use the new anti-warning facilities you just introduced?

But presumably this is a real warning about a function that will go away
(and the viper-ex function won't), so it should be fixed by rewriting
this in terms of the xref-* functions.

But I don't know how.  :-)

There's a handful more of similar calls in the Emacs tree.  So what is
the proper way to rewrite calls like (find-tag ex-tag t) in terms of the
new xref-* machinery?

I've Cc'd Dmitry on this now...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: viper-ex

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org, Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 13 Jun 2019 14:00:08 +0200
> 
> >> 	  (if (string= tag "")
> >> 	      (find-tag ex-tag t)
> >> 	    (find-tag-other-window ex-tag))
> >
> > Time to use the new anti-warning facilities you just introduced?
> 
> But presumably this is a real warning about a function that will go away
> (and the viper-ex function won't), so it should be fixed by rewriting
> this in terms of the xref-* functions.

I don't think find-tag is going anywhere any time soon.

Re: Towards a cleaner build: viper-ex

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't think find-tag is going anywhere any time soon.

So should we un-obsolete the function?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: viper-ex

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org,  dgutov@yandex.ru
> Date: Thu, 13 Jun 2019 14:47:07 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I don't think find-tag is going anywhere any time soon.
> 
> So should we un-obsolete the function?

Not yet, I still hope someone will step forward and code a suitable
replacement.  Or maybe there is such a replacement already and I'm
just missing something.

Re: Towards a cleaner build: viper-ex

[Dmitry Gutov]

On 13.06.2019 15:00, Lars Ingebrigtsen wrote:

> There's a handful more of similar calls in the Emacs tree.  So what is
> the proper way to rewrite calls like (find-tag ex-tag t) in terms of the
> new xref-* machinery?

There is no existing function with the same mechanics so far. But if 
it's important, we should be able to write something using the fileloop 
package.

Re: Towards a cleaner build: viper-ex

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> There is no existing function with the same mechanics so far. But if
> it's important, we should be able to write something using the
> fileloop package.

Yes, that would be nice.  Having only valid warnings in the Emacs build
would be a plus, I think, and those find-tags warnings have persisted in
the build for quite a while now...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: viper-ex

[Dmitry Gutov]

On 13.06.2019 15:54, Lars Ingebrigtsen wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> There is no existing function with the same mechanics so far. But if
>> it's important, we should be able to write something using the
>> fileloop package.
> 
> Yes, that would be nice.  Having only valid warnings in the Emacs build
> would be a plus, I think, and those find-tags warnings have persisted in
> the build for quite a while now...

It'll probably take more than a couple of hours, though, so maybe it's 
best to file a bug report.

Re: Towards a cleaner build

[Stefan Monnier]

>> `ignore` is a normal function used for various different things.
>> The fact that it can be used to silence this warning is just a happy
>> coincidence.
>
> Well, somebody surely has implemented it.  :-)

Nope.

> Somewhere.  I looked for the code and couldn't find it...

It's because there isn't any.

>>     grep "#'ignore" **/*.el | wc
>> gives me more than a hundred reasons not to mark it obsolete ;-)
> I meant the special construct; not the function...

There is no special construct:
`(ignore X Y Z)` is a normal function call, hence a use of X, Y, and Z.
It also happens that `ignore` is later optimized away so it usually has
no runtime cost (but that's only true when the code is compiled).


        Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> There is no special construct:
> `(ignore X Y Z)` is a normal function call, hence a use of X, Y, and Z.
> It also happens that `ignore` is later optimized away so it usually has
> no runtime cost (but that's only true when the code is compiled).

Oooh.  Huh.  Isn't that...  a bug?  :-)  I mean -- the reference to the
variable isn't in the compiled code, so shouldn't the analysis catch
that and still warn about the variable?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

> Oooh.  Huh.  Isn't that...  a bug?  :-)  I mean -- the reference to the
> variable isn't in the compiled code, so shouldn't the analysis catch
> that and still warn about the variable?

That's the usual tension with warnings: OT1H you want warnings to be
about the source code rather than about the compiled/optimized code, but
then you also don't want warning about things which the programmer knows
will be optimized away (e.g. within a (featurep 'xemacs) branch), so
ordering of "warning-detection" and optimization phases is
sometimes delicate.  For `ignore` things just happen to work.


        Stefan

Re: Towards a cleaner build

[Andreas Schwab]

On Jun 09 2019, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> (with-suppressed-warnings ((unused foo))
>   (defun bar (foo)
>     nil))

(defun bar (_foo) nil)

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: Towards a cleaner build

[Stefan Monnier]

> Or did you mean that that should return an intermediary function and
> then do the byte-hunk-handler thing as in my code sketch?
> *implements*
> Oh, wow, that worked.  :-)  It always helps to ask, apparently.

Always happy to help,


        Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Some of these warnings are perhaps not, like, very useful to be able to
suppress, but I thought it'd be nice if the warnings you could suppress
is the same as you can do via byte-compile-warnings, so I've implemented
almost all of them.

So now the following monstrosity compiles without a single warning.  :-)

;;; stest.el ---  -*- lexical-binding: t -*-

(require 'stest-defs)

(defun obsolete ()
  (declare (obsolete foo "22.1")))

(defun wrong-params (foo &optional _unused)
  foo)

(with-suppressed-warnings ((lexical prefixless))
  (defvar prefixless))

(with-suppressed-warnings ((redefine redefine))
  (defun redefine (foo bar)
    (cons foo bar)))

(defun tests ()
  (with-suppressed-warnings ((constants nil))
    (let ((nil t))
      (message-mail)))
  
  (with-suppressed-warnings ((obsolete obsolete))
    (obsolete))
  (with-suppressed-warnings ((callargs wrong-params))
    (wrong-params 1 2 3))
  (with-suppressed-warnings ((obsolete obsolete-variable))
    obsolete-variable)

  (with-suppressed-warnings ((mapcar mapcar))
    (mapcar #'list '(1 2 3)))
  
  (with-suppressed-warnings ((free-vars free-variable))
    free-variable)

  (with-suppressed-warnings ((suspicious set-buffer))
    (save-excursion
      (set-buffer (get-buffer-create "foo"))
      nil))

  )


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Noam Postavsky]

On Sun, 9 Jun 2019 at 11:40, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> (with-suppressed-warnings ((lexical prefixless))
>   (defvar prefixless))

It's a bit funny that the warning is named 'lexical', when it's about
a non-lexical variable.

>   (with-suppressed-warnings ((obsolete obsolete))
>     (obsolete))

>   (with-suppressed-warnings ((obsolete obsolete-variable))
>     obsolete-variable)

We might want to support suppressing the warning for obsolete
functions and variables separately?

>   (with-suppressed-warnings ((mapcar mapcar))
>     (mapcar #'list '(1 2 3)))

This is the 'mapcar called for effect' one, right? Could we group it
with 'value returned from (+ 1 2) is unused' for (progn (+ 1 2) nil)?
Even though the byte compiler implementation might have them seperate,
I think they're pretty much the same thing from the point of view of
an elisp author.

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> On Sun, 9 Jun 2019 at 11:40, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
>> (with-suppressed-warnings ((lexical prefixless))
>>   (defvar prefixless))
>
> It's a bit funny that the warning is named 'lexical', when it's about
> a non-lexical variable.
>
>>   (with-suppressed-warnings ((obsolete obsolete))
>>     (obsolete))
>
>>   (with-suppressed-warnings ((obsolete obsolete-variable))
>>     obsolete-variable)
>
> We might want to support suppressing the warning for obsolete
> functions and variables separately?
>
>>   (with-suppressed-warnings ((mapcar mapcar))
>>     (mapcar #'list '(1 2 3)))
>
> This is the 'mapcar called for effect' one, right? Could we group it
> with 'value returned from (+ 1 2) is unused' for (progn (+ 1 2) nil)?
> Even though the byte compiler implementation might have them seperate,
> I think they're pretty much the same thing from the point of view of
> an elisp author.

I agree with all these, but I thought it might be nice to have the same
symbols as in byte-compile-warnings (and byte-compile-warning-types),
which are:

=> (redefine callargs free-vars unresolved obsolete noruntime cl-functions interactive-only make-local mapcar constants suspicious lexical)

If we diverge featurewise between `byte-compile-warnings' and
`with-suppressed-warnings', I think that will be confusing.

But...  perhaps we could tweak `byte-compile-warnings' featurewise, too?
That is, split `obsolete' into `obsolete-function' and
`obsolete-variable' (and retain `obsolete' as a catch-all for both),
etc...  Perhaps that'd be nice?

`effect' would then be the new synonym for `mapcar'...  And we could
have `no-prefix' as a synonym for `lexical'.  Actually, I'd guess that
`lexical' was chosen because you only get that warning in files with
lexical binding, but it's kinda opaque that it has that effect.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Noam Postavsky]

On Sun, 9 Jun 2019 at 12:25, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> I agree with all these, but I thought it might be nice to have the same
> symbols as in byte-compile-warnings (and byte-compile-warning-types),

Oh, I missed that, yeah, that make sense.

> But...  perhaps we could tweak `byte-compile-warnings' featurewise, too?

But maybe do that in a separate step (i.e., after
with-suppressed-warnings lands on master).

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

>> But...  perhaps we could tweak `byte-compile-warnings' featurewise, too?
>
> But maybe do that in a separate step (i.e., after
> with-suppressed-warnings lands on master).

Makes sense.  Or...  perhaps we'd only want to allow the new symbols in
`with-suppressed-warnings' while we allow the legacy symbols in
`byte-compile-warnings'?  Then it'd make sense to do both at the same
time...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

>>   (with-suppressed-warnings ((mapcar mapcar))
>>     (mapcar #'list '(1 2 3)))
> This is the 'mapcar called for effect' one, right?

Hmm... I've never seen this warning misfire nor have I ever been unable
to replace the mapcar with mapc, so I'm not sure we care about this case
w.r.t with-suppressed-warnings.

> Could we group it with 'value returned from (+ 1 2) is unused' for
> (progn (+ 1 2) nil)?

I can't remember having to silence this warning either, FWIW.


        Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Hmm... I've never seen this warning misfire nor have I ever been unable
> to replace the mapcar with mapc, so I'm not sure we care about this case
> w.r.t with-suppressed-warnings.

No, these aren't very useful suppressions, but since they can be
silenced individually by setting byte-compile-warnings, I think it makes
sense to also allow with-suppressed-warnings to do the same.  Interface
regularity is nice.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

>> Hmm... I've never seen this warning misfire nor have I ever been unable
>> to replace the mapcar with mapc, so I'm not sure we care about this case
>> w.r.t with-suppressed-warnings.
>
> No, these aren't very useful suppressions, but since they can be
> silenced individually by setting byte-compile-warnings, I think it makes
> sense to also allow with-suppressed-warnings to do the same.  Interface
> regularity is nice.

I see no reason not to support them, indeed, but I just mean that
there's no point trying to spend any effort trying to fine-tune the
behavior in this case.


        Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Uhm.  All the other things work great here, but if I modify
display-completion-list to add a with-suppressed-warnings (whether it
has an effect or not; in the example below it's suppressing a warning
that won't be output), I get a severe compilation error.

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index ed610c16ee..f3182d31c0 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1738,7 +1738,8 @@ display-completion-list
       (with-temp-buffer
 	(let ((standard-output (current-buffer))
 	      (completion-setup-hook nil))
-	  (display-completion-list completions common-substring))
+          (with-suppressed-warnings ((interactive-only display-completion-list))
+	    (display-completion-list completions common-substring)))
 	(princ (buffer-string)))
 
     (with-current-buffer standard-output


Here's the error:

make[1]: Entering directory '/home/larsi/src/emacs/with-suppressed-warnings/lisp'
  ELC      minibuffer.elc

In display-completion-list:
minibuffer.el:1742:14:Warning: display-completion-list called with 2
    arguments, but accepts only 1
minibuffer.el:1750:10:Error: Compiler bug: depth conflict at tag 3
Makefile:296: recipe for target 'minibuffer.elc' failed
make[1]: *** [minibuffer.elc] Error 1
make[1]: Leaving directory '/home/larsi/src/emacs/with-suppressed-warnings/lisp'
Makefile:319: recipe for target 'compile-main' failed
make: *** [compile-main] Error 2

Uhm...  anybody want to hazard a guess at why it fails or where I should
poke around to see what I've messed up?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

make[1]: Entering directory '/home/larsi/src/emacs/with-suppressed-warnings/lisp'
  ELC      minibuffer.elc

In toplevel form:
minibuffer.el:1719:1:Warning: Unused lexical variable ‘foo’
minibuffer.el:1719:1:Warning: Unused lexical argument ‘completions’
minibuffer.el:1723:17:Error: Assertion failed: (<= 0 (+ opcode off))
Makefile:296: recipe for target 'minibuffer.elc' failed
make[1]: *** [minibuffer.elc] Error 1
make[1]: Leaving directory '/home/larsi/src/emacs/with-suppressed-warnings/lisp'
Makefile:319: recipe for target 'compile-main' failed
make: *** [compile-main] Error 2

Curioser and curioser.  I get the error above with this:

(defun display-completion-list (completions &optional common-substring)
  (with-suppressed-warnings ((callargs baz))
    (foo))
  (let ((foo common-substring))
    (run-hooks 'zot)))

If I change common-substring in the let to something else, or remove the
run-hooks, then there's no error...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> minibuffer.el:1723:17:Error: Assertion failed: (<= 0 (+ opcode off))

Never mind -- I didn't understand how things worked in this area with
regards to side effects versus compilation artefacts.  Well, I still
don't, but I stared at other bits of the byte compiler and apparently
got it to work...  

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

> Not sure, but after spending time on preventing defsetf from defining
> things in the compiler's Emacs session[1], I would feel bad about
> letting this one through ;)
>
> [1]: https://debbugs.gnu.org/27016

BTW, obsolescence annotations are stored as symbol properties, just like
the defsetf thingy, so it could use byte-compile-plist-environment
as well.


        Stefan

Re: Towards a cleaner build

[Stefan Monnier]

> But this means that as of present, if you have an obsolete function in a
> file, and you call it from a function defined before that obsolete
> function, you won't get the warning, so this seems like a bug...

Note that if a function/variable FOO is defined as obsolete in file BAR, then
I think we'd ideally want to silence all warnings for uses of FOO made
within BAR.


        Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Note that if a function/variable FOO is defined as obsolete in file BAR, then
> I think we'd ideally want to silence all warnings for uses of FOO made
> within BAR.

Determining the current file is easy enough (with
`byte-compile-current-file'), but what's the best way to determine what
file that obsolete function came from?

`find-lisp-object-file-name' seems pretty heavy handed...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

>> Note that if a function/variable FOO is defined as obsolete in file BAR, then
>> I think we'd ideally want to silence all warnings for uses of FOO made
>> within BAR.
> Determining the current file is easy enough (with
> `byte-compile-current-file'), but what's the best way to determine what
> file that obsolete function came from?

I'm not suggesting we encode the above condition in the code.
As I said elsewhere, I think trying to be too clever in which warnings
to automatically silence quickly gets to diminishing returns.

More important is the support for the author to be able to silence those
warnings which are false positives since that works regardless if we could
come up with a way to detect it automatically or not.


        Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Note that if a function/variable FOO is defined as obsolete in file BAR, then
> I think we'd ideally want to silence all warnings for uses of FOO made
> within BAR.

But I'm not quite sure this is ideal -- someone may well mark something
as obsolete in BAR, and then meant to fix the call sites for FOO
throughout BAR, and then it'd be useful to get the byte compiler to tell
you where the problematic calls are.

So I think this would suppress too many useful warnings, whereas
suppressing warnings about obsolete functions calling each other is much
less problematic.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Lars Ingebrigtsen]

I had a gander.  :-)

It doesn't really look that complicated.  So we'd introduce a new
function with-suppressed-warnings along the lines of with-no-warnings:

(byte-defop-compiler-1 with-no-warnings byte-compile-no-warnings)
(defun byte-compile-no-warnings (form)
  (let (byte-compile-warnings)
    (byte-compile-form (cons 'progn (cdr form)))))

And the warnings are issued in contexts like this:

(defun byte-compile-warn-obsolete (symbol)
  "Warn that SYMBOL (a variable or function) is obsolete."
  (when (byte-compile-warning-enabled-p 'obsolete)
    ...))

So that would change to

   (byte-compile-warning-enabled-p 'obsolete symbol)

and the main logic for seeing whether we want to warn for that obsolete
symbol (etc) would basically be all constrained to the
`byte-compile-warning-enabled-p' function?

So if we want to go this direction it looks like it can be done in a
pretty clean fashion.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

> So if we want to go this direction it looks like it can be done in a
> pretty clean fashion.

Pretty please?


        Stefan

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> So if we want to go this direction it looks like it can be done in a
>> pretty clean fashion.
>
> Pretty please?

I made a proof of concept thing by just cargo-culting the code for
with-no-warnings, and it works!

I used the ehelp.el warning to test with

  (with-suppressed-warnings ((interactive-only execute-extended-command))
    (setq electric-help-form-to-execute
          (lambda () (execute-extended-command nil))))

and now it compiles cleanly and even seems to work!?  Both interactively
and byte-compiled, although more testing is required.

There's some details in the interface that could be bike-shedded...  for
instance, how often do we need several things suppressed at once?  If we
limit it to one thing at a time, then we lose one set of parentheses: 

  (with-suppressed-warnings (interactive-only execute-extended-command)
    ...)

In the rare cases you need more, you can say

  (with-suppressed-warnings (obsolete something-else)
    (with-suppressed-warnings (interactive-only execute-extended-command)
      ...))

You can also either just allow one symbol to be suppressed per warning
type, or more, and the implementation supports the latter, so you can
say:

  (with-suppressed-warnings ((obsolete something something-else))
    ...)

I think that makes sense, and it doesn't add more (or less) visual
clutter..

Eli, what do you think?  Is `with-suppressed-warnings' something you can
see Emacs having?  If so, I can finish it up and write some
documentation...


diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index 842d1d48b4..9a9d93367b 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -494,6 +494,18 @@ with-no-warnings
   ;; The implementation for the interpreter is basically trivial.
   (car (last body)))
 
+(defmacro with-suppressed-warnings (warnings &rest body)
+  "Like `progn', but prevents compiler warnings in the body."
+  (declare (indent 1))
+  ;; The implementation for the interpreter is basically trivial.
+  `(with-suppressed-warnings-1 ',warnings (progn ,@body)))
+
+(defun with-suppressed-warnings-1 (_ &rest body)
+  "Like `progn', but prevents compiler warnings in the body."
+  (declare (indent 1))
+  ;; The implementation for the interpreter is basically trivial.
+  (car (last body)))
+
 \f
 (defun byte-run--unescaped-character-literals-warning ()
   "Return a warning about unescaped character literals.
diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index e76baf5ed0..1285ef9037 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -331,18 +331,26 @@ byte-compile-warnings
                       ,@(mapcar (lambda (x) `(const ,x))
                                 byte-compile-warning-types))))
 
+(defvar byte-compile-suppressed-warnings nil)
+
 ;;;###autoload
 (put 'byte-compile-warnings 'safe-local-variable
      (lambda (v)
        (or (symbolp v)
            (null (delq nil (mapcar (lambda (x) (not (symbolp x))) v))))))
 
-(defun byte-compile-warning-enabled-p (warning)
+(defun byte-compile-warning-enabled-p (warning &optional symbol)
   "Return non-nil if WARNING is enabled, according to `byte-compile-warnings'."
-  (or (eq byte-compile-warnings t)
-      (if (eq (car byte-compile-warnings) 'not)
-          (not (memq warning byte-compile-warnings))
-        (memq warning byte-compile-warnings))))
+  (let ((suppress nil))
+    (dolist (elem byte-compile-suppressed-warnings)
+      (when (and (eq (car elem) warning)
+                 (memq symbol (cdr elem)))
+        (setq suppress t)))
+    (and (not suppress)
+         (or (eq byte-compile-warnings t)
+             (if (eq (car byte-compile-warnings) 'not)
+                 (not (memq warning byte-compile-warnings))
+               (memq warning byte-compile-warnings))))))
 
 ;;;###autoload
 (defun byte-compile-disable-warning (warning)
@@ -2518,6 +2526,15 @@ byte-compile-file-form-with-no-warnings
     (mapc 'byte-compile-file-form (cdr form))
     nil))
 
+(put 'with-suppressed-warnings-1 'byte-hunk-handler
+     'byte-compile-file-form-with-suppressed-warnings)
+(defun byte-compile-file-form-with-suppressed-warnings (form)
+  ;; cf byte-compile-file-form-progn.
+  (let ((byte-compile-suppressed-warnings
+         (append (cadadr form) byte-compile-suppressed-warnings)))
+    (mapc 'byte-compile-file-form (cddr form))
+    nil))
+
 ;; Automatically evaluate define-obsolete-function-alias etc at top-level.
 (put 'make-obsolete 'byte-hunk-handler 'byte-compile-file-form-make-obsolete)
 (defun byte-compile-file-form-make-obsolete (form)
@@ -3150,7 +3167,7 @@ byte-compile-form
         (when (and (byte-compile-warning-enabled-p 'suspicious)
                    (macroexp--const-symbol-p fn))
           (byte-compile-warn "`%s' called as a function" fn))
-	(when (and (byte-compile-warning-enabled-p 'interactive-only)
+	(when (and (byte-compile-warning-enabled-p 'interactive-only fn)
 		   interactive-only)
 	  (byte-compile-warn "`%s' is for interactive use only%s"
 			     fn
@@ -4765,6 +4782,14 @@ byte-compile-no-warnings
   (let (byte-compile-warnings)
     (byte-compile-form (cons 'progn (cdr form)))))
 
+(byte-defop-compiler-1 with-suppressed-warnings-1
+                       byte-compile-suppressed-warnings)
+(defun byte-compile-suppressed-warnings (form)
+  (let ((byte-compile-suppressed-warnings
+         (append (cadadr form) byte-compile-suppressed-warnings)))
+    (mapc 'byte-compile-file-form (cddr form))
+    nil))
+
 ;; Warn about misuses of make-variable-buffer-local.
 (byte-defop-compiler-1 make-variable-buffer-local
                        byte-compile-make-variable-buffer-local)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Fri, 17 May 2019 07:25:01 -0400
> Cc: Eli Zaretskii <eliz@gnu.org>, Stefan Monnier <monnier@iro.umontreal.ca>, 
> 	Emacs developers <emacs-devel@gnu.org>
> 
> >   (declare 'warning-about-arguments-mismatch 'display-completion-list 'off)
> >
> > perhaps?
> 
> This makes sense, but it should come for free. There's never any point
> in issuing a warning about not using the advertised calling convention
> from inside the function itself. Similarly, we shouldn't issue
> warnings about calls to obsolete functions from bodies of functions
> that are themselves obsolete.

I tend to agree.

Re: Towards a cleaner build

[Stefan Monnier]

> This makes sense, but it should come for free. There's never any point
> in issuing a warning about not using the advertised calling convention
> from inside the function itself. Similarly, we shouldn't issue
> warnings about calls to obsolete functions from bodies of functions
> that are themselves obsolete.

I generally agree, but there's a tension: this kind of reasoning can be
pushed pretty far and the amount of coding involved can be hard to
justify.  So there's also something to be said for leaving the
implementation dumb and asking the coder to add a few more
with-(no|fewer|suppressed)-warnings.

IOW whether it's worth the effort depends on the amount of complexity
needed in the compiler and the number of false-positives it eliminates
(and potentially the number of false-negatives it adds, tho this is
rarely a problem).


        Stefan

RE: Towards a cleaner build

[Drew Adams]

> In Info-edit:
> info.el:4415:4:Warning: ‘Info-edit-mode’ is an obsolete function (as of 24.4);
>     editing Info nodes by hand is not recommended.
> 
> which is
> (make-obsolete 'Info-edit-mode
> 	       "editing Info nodes by hand is not recommended." "24.4")
> 
> (defun Info-edit ()
>   "Edit the contents of this Info node."
>   (interactive)
>   (Info-edit-mode)
>   (message "%s" (substitute-command-keys
> 		 "Editing: Type \\<Info-edit-mode-map>\\[Info-cease-edit] to
> return to info")))
> 
> (put 'Info-edit 'disabled "Editing Info nodes by hand is not recommended.
> This feature will be removed in future.")
> 
> (make-obsolete 'Info-edit
> 	       "editing Info nodes by hand is not recommended." "24.4")
> 
> etc etc etc.  All the Info-edit stuff was declared obsolete in 24.4, and
> I seem to recall that our policy for removing obsolete stuff is two
> release cycles?  (I couldn't find anything in admin/notes that said
> anything about this explicitly...)  So it OK to remove all the Info-edit
> modes and commands now?

FWIW, I don't think we should have declared it "obsolete".
It's OK to recommend not editing nodes, but I don't see
why we would remove the possibility.  It's a quick-and-easy
(or quick-and-dirty, if you prefer) way for a user to add
her own notes or clarifications to the existing doc.  So
what if it's temporary, hackish, not texinfo etc.?  Has its
existence ever hurt anyone?  Is it a pain to "maintain"?

Yes, I said this at the time.  Just one opinion.  Does no
harm, can be somewhat useful.  Just remove the runtime
warning and leave it disabled by default: voila - "cleaner
build."

Re: Towards a cleaner build

[Lars Ingebrigtsen]

In xselect--encode-string:
select.el:475:24:Warning: ‘string-make-unibyte’ is an obsolete function (as of
    26.1); use ‘encode-coding-string’.

This is the code:

	   ((eq type 'C_STRING)
	    (setq str (string-make-unibyte str)))

(string-make-unibyte "á☪foo")
=> "\341*foo"

I'm guessing the point here is to make an 8-bit string that is somewhat
congruent with the (possibly) multibyte string that we have here?  Do we
have a library function (or a coding system) that does the same thing?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 17 May 2019 05:32:58 +0200
> 
> In xselect--encode-string:
> select.el:475:24:Warning: ‘string-make-unibyte’ is an obsolete function (as of
>     26.1); use ‘encode-coding-string’.
> 
> This is the code:
> 
> 	   ((eq type 'C_STRING)
> 	    (setq str (string-make-unibyte str)))
> 
> (string-make-unibyte "á☪foo")
> => "\341*foo"

This example is not relevant, because C_STRING is for strings that are
actually binary blobs, i.e. sequences of raw bytes.  If you use
examples of human-readable text, you get the wrong impression of what
is intended.

> I'm guessing the point here is to make an 8-bit string that is somewhat
> congruent with the (possibly) multibyte string that we have here?

The point here is to convert our internal multibyte representation of
raw bytes into their external single-byte representation.

> Do we have a library function (or a coding system) that does the
> same thing?

Yes, it's called string-make-unibyte ;-)

I'm not sure we have anything else, but let me dwell on this for a
while.

Btw, your other change in select.el was incorrect, and I reverted it.
For starters, when we need to _decode_ text (which is what
gui-get-selection does, since it imports text into Emacs), it can
never be TRT to _encode_ it.  And second, there's no such
coding-system as 'eight-bit', that's a name of a charset.

  (coding-system-p 'eight-bit) => nil

So something like (encode-coding-string FOO 'eight-bit) will do
nothing useful, and just signal an error.

In general (with the possible exception of Gnus ;-), the places where
we left those string-make/to/as-uni/multibyte thingies are there not
because we were lazy, but because the replacements are either not
trivial or don't exist.  The deprecation message's advice in most of
these cases is simply misleading.  So I welcome work on these places,
but my advice is to discuss each one of them before making changes,
lest we break the code in some more or less subtle use cases.

Thanks.

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> 	   ((eq type 'C_STRING)
>> 	    (setq str (string-make-unibyte str)))
>> 
>> (string-make-unibyte "á☪foo")
>> => "\341*foo"
>
> This example is not relevant, because C_STRING is for strings that are
> actually binary blobs, i.e. sequences of raw bytes.  If you use
> examples of human-readable text, you get the wrong impression of what
> is intended.

Yeah, I was looking at the doc string for that function, which begins

---
Multibyte character codes are converted to unibyte according to
‘nonascii-translation-table’ or, if that is nil, ‘nonascii-insert-offset’.
---

which makes it sound like it's made for doing human-readable translation
somehow.  And I was fooled by ☪ happening to have * as the lower byte,
so it looked that way, too.  :-)

But I see that you've now removed that bit from the doc string, so now
it makes more sense.

>> Do we have a library function (or a coding system) that does the
>> same thing?
>
> Yes, it's called string-make-unibyte ;-)
>
> I'm not sure we have anything else, but let me dwell on this for a
> while.

Ok.  :-)

> Btw, your other change in select.el was incorrect, and I reverted it.
> For starters, when we need to _decode_ text (which is what
> gui-get-selection does, since it imports text into Emacs), it can
> never be TRT to _encode_ it.

Oh, sorry; thinko.

> In general (with the possible exception of Gnus ;-), the places where
> we left those string-make/to/as-uni/multibyte thingies are there not
> because we were lazy, but because the replacements are either not
> trivial or don't exist.

Right.  Should we slap a with-no-warnings around this call, then?
Warning about a usage where we have verified that it's what we really,
really want to do seems counter productive...

                     ;; We want to convert each non-ASCII byte to the
                     ;; corresponding eight-bit character, which has
                     ;; a codepoint >= #x3FFF00.
                     (string-to-multibyte data))))

> The deprecation message's advice in most of
> these cases is simply misleading.  So I welcome work on these places,
> but my advice is to discuss each one of them before making changes,
> lest we break the code in some more or less subtle use cases.

Will do.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Fri, 17 May 2019 12:24:40 +0200
> 
> > Yes, it's called string-make-unibyte ;-)
> >
> > I'm not sure we have anything else, but let me dwell on this for a
> > while.
> 
> Ok.  :-)
> 
> > Btw, your other change in select.el was incorrect, and I reverted it.
> > For starters, when we need to _decode_ text (which is what
> > gui-get-selection does, since it imports text into Emacs), it can
> > never be TRT to _encode_ it.
> 
> Oh, sorry; thinko.
> 
> > In general (with the possible exception of Gnus ;-), the places where
> > we left those string-make/to/as-uni/multibyte thingies are there not
> > because we were lazy, but because the replacements are either not
> > trivial or don't exist.
> 
> Right.  Should we slap a with-no-warnings around this call, then?

Maybe, but let me dwell on that as well.

> > The deprecation message's advice in most of
> > these cases is simply misleading.  So I welcome work on these places,
> > but my advice is to discuss each one of them before making changes,
> > lest we break the code in some more or less subtle use cases.
> 
> Will do.

Thanks.

Re: Towards a cleaner build

[Lars Ingebrigtsen]

In ps-begin-job:
ps-print.el:5773:17:Warning: ‘string-as-unibyte’ is an obsolete function (as
    of 26.1); use ‘encode-coding-string’.
ps-print.el:5775:17:Warning: ‘string-as-unibyte’ is an obsolete function (as
    of 26.1); use ‘encode-coding-string’.

Here's the code:

	(cond ((eq ps-print-control-characters '8-bit)
	       (string-as-unibyte "[\000-\037\177-\377]"))
	      ((eq ps-print-control-characters 'control-8-bit)
	       (string-as-unibyte "[\000-\037\177-\237]"))
	      ((eq ps-print-control-characters 'control)
	       "[\000-\037\177]")
	      (t "[\t\n\f]"))

But...  aren't both those strings unibyte already?  Am I missing
something?  The code is from 1998, so that's in Mule territory, I guess,
so perhaps they weren't then...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 17 May 2019 05:51:37 +0200
> 
> In ps-begin-job:
> ps-print.el:5773:17:Warning: ‘string-as-unibyte’ is an obsolete function (as
>     of 26.1); use ‘encode-coding-string’.
> ps-print.el:5775:17:Warning: ‘string-as-unibyte’ is an obsolete function (as
>     of 26.1); use ‘encode-coding-string’.
> 
> Here's the code:
> 
> 	(cond ((eq ps-print-control-characters '8-bit)
> 	       (string-as-unibyte "[\000-\037\177-\377]"))
> 	      ((eq ps-print-control-characters 'control-8-bit)
> 	       (string-as-unibyte "[\000-\037\177-\237]"))
> 	      ((eq ps-print-control-characters 'control)
> 	       "[\000-\037\177]")
> 	      (t "[\t\n\f]"))
> 
> But...  aren't both those strings unibyte already?

The question is not whether the original strings are unibyte, the
question is does

  (setq foo "[\000-\037\177-\377]")

produce a unibyte or a multibyte string, including after processing by
the Lisp reader (and when this file is byte-compiled).  E.g., \377 is
above decimal 127, so it could, in principle, be interpreted as the
corresponding Unicode codepoint of a Latin character.

If you verified that removing string-as-unibyte here produces a
unibyte string in ps-control-or-escape-regexp, including after
byte-compilation, then yes, we can remove that function call here.
But for making this future-proof, I'd add an assertion there, or maybe
add a test to verify this stays that way, come what may.

Thanks.

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> The question is not whether the original strings are unibyte, the
> question is does
>
>   (setq foo "[\000-\037\177-\377]")
>
> produce a unibyte or a multibyte string, including after processing by
> the Lisp reader (and when this file is byte-compiled).

Let's see...  I put this in ps-print.el:

(defun ps-test ()
  (let ((string1 "[\000-\037\177-\377]")
        (string2 "[\000-\037\177-\237]"))
    (message "%s %s\n%s %s"
             (multibyte-string-p string1) string1
             (multibyte-string-p string2) string2)))

(ps-test)
=> "nil [^@-^_^?-\377]
nil [^@-^_^?-\237]"

(load "~/src/emacs/trunk/lisp/ps-print.el")
(ps-test)
=> "nil [^@-^_^?-\377]
nil [^@-^_^?-\237]"

(byte-compile-file "~/src/emacs/trunk/lisp/ps-print.el")
(load "~/src/emacs/trunk/lisp/ps-print.elc")
(ps-test)
"nil [^@-^_^?-\377]
nil [^@-^_^?-\237]"

ps-test is a compiled Lisp function in ‘ps-print.el’.

(ps-test)

So they do seem to be unibyte no matter how I load/eval the function,
unless I did something wrong here.

> But for making this future-proof, I'd add an assertion there, or maybe
> add a test to verify this stays that way, come what may.

OK; I'll remove and add an assertion.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 17 May 2019 04:15:46 +0200
> 
> (put 'Info-edit 'disabled "Editing Info nodes by hand is not recommended.
> This feature will be removed in future.")
> 
> (make-obsolete 'Info-edit
> 	       "editing Info nodes by hand is not recommended." "24.4")
> 
> 
> etc etc etc.  All the Info-edit stuff was declared obsolete in 24.4, and
> I seem to recall that our policy for removing obsolete stuff is two
> release cycles?  (I couldn't find anything in admin/notes that said
> anything about this explicitly...)  So it OK to remove all the Info-edit
> modes and commands now?

Would it make sense instead to move it to a separate file and put that
file in lisp/obsolete?

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Would it make sense instead to move it to a separate file and put that
> file in lisp/obsolete?

I can do that if you think that's the right solution.  But it's been
obsolete since 2013, so isn't it time for it to go?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Fri, 17 May 2019 07:52:13 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Would it make sense instead to move it to a separate file and put that
> > file in lisp/obsolete?
> 
> I can do that if you think that's the right solution.  But it's been
> obsolete since 2013, so isn't it time for it to go?

Moving it into obsolete is a way of making it "go", no?

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Would it make sense instead to move it to a separate file and put that
>> > file in lisp/obsolete?
>> 
>> I can do that if you think that's the right solution.  But it's been
>> obsolete since 2013, so isn't it time for it to go?
>
> Moving it into obsolete is a way of making it "go", no?

But we do eventually remove obsolete stuff from lisp/obsolete too,
right?  :-)

I'm just wondering whether there's a policy on these things so that I
don't have to bother you with these details -- a period of time, or a
number of releases, or something, that determines when code marked as
obsolete should be deleted from the Emacs source tree.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Fri, 17 May 2019 12:19:08 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Would it make sense instead to move it to a separate file and put that
> >> > file in lisp/obsolete?
> >> 
> >> I can do that if you think that's the right solution.  But it's been
> >> obsolete since 2013, so isn't it time for it to go?
> >
> > Moving it into obsolete is a way of making it "go", no?
> 
> But we do eventually remove obsolete stuff from lisp/obsolete too,
> right?  :-)

Eventually.

> I'm just wondering whether there's a policy on these things so that I
> don't have to bother you with these details -- a period of time, or a
> number of releases, or something, that determines when code marked as
> obsolete should be deleted from the Emacs source tree.

We have something like a policy, but AFAIR this case is special: we
obsolete a user command on the premises that it is no longer used, and
don't provide any replacement.  Which is why I think obsolete/ might
be a better place for it.

Re: Towards a cleaner build

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> We have something like a policy, but AFAIR this case is special: we
> obsolete a user command on the premises that it is no longer used, and
> don't provide any replacement.  Which is why I think obsolete/ might
> be a better place for it.

OK; I'll move the editing bits to lisp/obsolete/info-edit.el.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build

[Stefan Monnier]

> We have something like a policy, but AFAIR this case is special: we
> obsolete a user command on the premises that it is no longer used, and
> don't provide any replacement.  Which is why I think obsolete/ might
> be a better place for it.

I think in cases where we think there might still be occasional users,
lisp/obsolete files can be moved to GNU ELPA.


        Stefan

Re: Towards a cleaner build: arc-mode

[Lars Ingebrigtsen]

This is the most head-scratchiest of all the string-to-multibyte stuff.

(eval-when-compile
  (defsubst byte-after (pos)
    "Like char-after but an eight-bit char is converted to unibyte."
    (multibyte-char-to-unibyte (char-after pos)))
  (defsubst insert-unibyte (&rest args)
    "Like insert but don't make unibyte string and eight-bit char multibyte."
    (dolist (elt args)
      (if (integerp elt)
	  (insert (if (< elt 128) elt (decode-char 'eight-bit elt)))
	(insert (string-to-multibyte elt)))))
  )

First of all, it defines defsubsts in the global namespace, but
(decode-char 'eight-bit 130) => 4194178

Hm...  Oh!  Is it working in a multibyte buffer, so it wants to insert
bytes that are in Emacs' "byte plane"?

(insert (decode-char 'eight-bit 130))
=> \202

I guess.  Well, that's one way of doing it...  er...  I think...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: arc-mode

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 17 May 2019 12:35:47 +0200
> 
> This is the most head-scratchiest of all the string-to-multibyte stuff.
> 
> (eval-when-compile
>   (defsubst byte-after (pos)
>     "Like char-after but an eight-bit char is converted to unibyte."
>     (multibyte-char-to-unibyte (char-after pos)))
>   (defsubst insert-unibyte (&rest args)
>     "Like insert but don't make unibyte string and eight-bit char multibyte."
>     (dolist (elt args)
>       (if (integerp elt)
> 	  (insert (if (< elt 128) elt (decode-char 'eight-bit elt)))
> 	(insert (string-to-multibyte elt)))))
>   )

We have get-byte nowadays, so I believe the first defsubst is not
needed.  You just need to catch errors, maybe.

> First of all, it defines defsubsts in the global namespace, but
> (decode-char 'eight-bit 130) => 4194178
> 
> Hm...  Oh!  Is it working in a multibyte buffer, so it wants to insert
> bytes that are in Emacs' "byte plane"?
> 
> (insert (decode-char 'eight-bit 130))
> => \202
> 
> I guess.

Yes.  However, I suspect this juggling is no longer need nowadays.  If
you just use 'insert', does arc-mode break when you invoke the
commands that end up using these calls?  This should be tried also in
a locale whose codeset is not UTF-8.

Thanks.

Re: Towards a cleaner build: arc-mode

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> We have get-byte nowadays, so I believe the first defsubst is not
> needed.  You just need to catch errors, maybe.

[...]

> Yes.  However, I suspect this juggling is no longer need nowadays.  If
> you just use 'insert', does arc-mode break when you invoke the
> commands that end up using these calls?  This should be tried also in
> a locale whose codeset is not UTF-8.

I'm wondering whether arc-mode.el is maintained, because it seems rather
er odd, and when I use the functions that actually use the library
functions we want to change, I'm getting bug-outs.

For instance, when renaming a file name in an .lzh archive:

Debugger entered--Lisp error: (end-of-buffer)
  delete-char(249)
  (let* ((p (+ archive-proper-file-start (aref descr 4))) (oldhsize (byte-after p)) (oldfnlen (byte-after (+ p 21))) (newfnlen (length newname)) (newhsize (+ oldhsize newfnlen (- oldfnlen))) (inhibit-read-only t)) (if (> newhsize 255) (error "The file name is too long")) (goto-char (+ p 21)) (delete-char (1+ oldfnlen)) (insert-unibyte newfnlen newname) (goto-char p) (delete-char 2) (insert-unibyte newhsize (archive-lzh-resum p newhsize)))

[...]

  archive-lzh-rename-entry("text2" ["text.txt" "text.txt" nil nil 930])

So finding code in there that works to check whether the changes we want
to make are sound is proving a challenge.  :-)

Hm...  here's the matrix of supported functionality:

;;			Arc	Lzh	Zip	Zoo	Rar	7z
;;			--------------------------------------------
;; View listing		Intern	Intern	Intern	Intern	Y	Y
;; Extract member	Y	Y	Y	Y	Y	Y
;; Save changed member	Y	Y	Y	Y	N	Y
;; Add new member	N	N	N	N	N	N
;; Delete member	Y	Y	Y	Y	N	Y
;; Rename member	Y	Y	N	N	N	N
;; Chmod		-	Y	Y	-	N	N
;; Chown		-	Y	-	-	N	N
;; Chgrp		-	Y	-	-	N	N

Renaming files is only supported in .arc and .lzh files...  neither of
which are used much any more.  Perhaps those should be marked as
obsolete (especially since at least one of them don't work any more)?

Let's see...  `archive-zip-chmod-entry' actually seems to work, and uses
`insert-unibyte'...  but only on numbers, not strings, so I don't know
how to test the string part.  *scratches head*

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: arc-mode

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> We have get-byte nowadays, so I believe the first defsubst is not
>> needed.  You just need to catch errors, maybe.

For byte-after, I tried this:

(defun byte-after (pos)
  "Like char-after but an eight-bit char is converted to unibyte."
  (cl-assert (= (multibyte-char-to-unibyte (char-after pos))
                (get-byte pos)))
  (get-byte pos))

I then opened some archives (which will use byte-after all over the
place), and I got no assertion errors.

If you think it's OK, I can just replace all the calls to byte-after
with get-byte in arc-mode.el and remove the defsubst. 

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: arc-mode

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Sat, 18 May 2019 06:04:33 +0200
> 
> >> We have get-byte nowadays, so I believe the first defsubst is not
> >> needed.  You just need to catch errors, maybe.
> 
> For byte-after, I tried this:
> 
> (defun byte-after (pos)
>   "Like char-after but an eight-bit char is converted to unibyte."
>   (cl-assert (= (multibyte-char-to-unibyte (char-after pos))
>                 (get-byte pos)))
>   (get-byte pos))
> 
> I then opened some archives (which will use byte-after all over the
> place), and I got no assertion errors.
> 
> If you think it's OK, I can just replace all the calls to byte-after
> with get-byte in arc-mode.el and remove the defsubst. 

Yes, I think that would be fine, thanks.

Re: Towards a cleaner build: arc-mode

[Lars Ingebrigtsen]

There's one additional use of string-as-multibyte in arc-mode that I
didn't notice before:

(defun archive-l-e (str &optional len)
  "Convert little endian string/vector STR to integer.
Alternatively, STR may be a buffer position in the current buffer
in which case a second argument, length LEN, should be supplied."
  (if (stringp str)
      (setq len (length str))
    (setq str (buffer-substring str (+ str len))))
  (setq str (string-as-unibyte str))
  (let ((result 0)
        (i 0))
    (while (< i len)
      (setq i (1+ i)
            result (+ (ash result 8)
		      (aref str (- len i)))))
    result))

Hm...  These buffers are multibyte, but do not contain any characters,
just ASCII and stuff from the "byte plane"...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: arc-mode

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Sat, 18 May 2019 08:04:25 +0200
> 
> There's one additional use of string-as-multibyte in arc-mode that I
> didn't notice before:
> 
> (defun archive-l-e (str &optional len)
>   "Convert little endian string/vector STR to integer.
> Alternatively, STR may be a buffer position in the current buffer
> in which case a second argument, length LEN, should be supplied."
>   (if (stringp str)
>       (setq len (length str))
>     (setq str (buffer-substring str (+ str len))))
>   (setq str (string-as-unibyte str))
>   (let ((result 0)
>         (i 0))
>     (while (< i len)
>       (setq i (1+ i)
>             result (+ (ash result 8)
> 		      (aref str (- len i)))))
>     result))

I think using this instead of string-as-unibyte should be OK:

  (if (multibyte-string-p str)
      (setq str (encode-coding-string str 'utf-8-emacs-unix)))

What string-as-unibyte does is it makes a unibyte string with the same
bytes as the internal representation of STR, except for raw bytes,
which it converts to their external single-byte form.  Encoding with
utf-8-emacs does the same.

Re: Towards a cleaner build: arc-mode

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I think using this instead of string-as-unibyte should be OK:
>
>   (if (multibyte-string-p str)
>       (setq str (encode-coding-string str 'utf-8-emacs-unix)))

I've now made this change.

> What string-as-unibyte does is it makes a unibyte string with the same
> bytes as the internal representation of STR, except for raw bytes,
> which it converts to their external single-byte form.  Encoding with
> utf-8-emacs does the same.

Ah, I see.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: arc-mode

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Sat, 18 May 2019 05:56:04 +0200
> 
> I'm wondering whether arc-mode.el is maintained, because it seems rather
> er odd, and when I use the functions that actually use the library
> functions we want to change, I'm getting bug-outs.

It's maintained, but old and largely unused archive formats bit-rot
with time.

>   archive-lzh-rename-entry("text2" ["text.txt" "text.txt" nil nil 930])
> 
> So finding code in there that works to check whether the changes we want
> to make are sound is proving a challenge.  :-)
> 
> Hm...  here's the matrix of supported functionality:
> 
> ;;			Arc	Lzh	Zip	Zoo	Rar	7z
> ;;			--------------------------------------------
> ;; View listing		Intern	Intern	Intern	Intern	Y	Y
> ;; Extract member	Y	Y	Y	Y	Y	Y
> ;; Save changed member	Y	Y	Y	Y	N	Y
> ;; Add new member	N	N	N	N	N	N
> ;; Delete member	Y	Y	Y	Y	N	Y
> ;; Rename member	Y	Y	N	N	N	N
> ;; Chmod		-	Y	Y	-	N	N
> ;; Chown		-	Y	-	-	N	N
> ;; Chgrp		-	Y	-	-	N	N
> 
> Renaming files is only supported in .arc and .lzh files...  neither of
> which are used much any more.  Perhaps those should be marked as
> obsolete (especially since at least one of them don't work any more)?
> 
> Let's see...  `archive-zip-chmod-entry' actually seems to work, and uses
> `insert-unibyte'...  but only on numbers, not strings, so I don't know
> how to test the string part.  *scratches head*

If zip archives work with the change, I suggest to make the change,
leave behind a FIXME comment regarding those which don't seem to work,
and file a bug report about that.  I will try to get to that when I
have time, if no one beats me to it.

Thanks.

Re: Towards a cleaner build: arc-mode

[Stefan Monnier]

> I'm wondering whether arc-mode.el is maintained,

I believe with all the changes you've made to it lately, you're the
maintainer.  I take advantage of this opportunity to mention that
arc-mode should be changed so as to stop mixing "bytes" and "chars":
move the bytes to a separate buffer (e.g. with buffer-swap-text) and
only keep in the "official" buffer the content-listing.


        Stefan

Re: Towards a cleaner build: arc-mode

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I'm wondering whether arc-mode.el is maintained,
>
> I believe with all the changes you've made to it lately, you're the
> maintainer.

:-)

> I take advantage of this opportunity to mention that arc-mode should
> be changed so as to stop mixing "bytes" and "chars": move the bytes to
> a separate buffer (e.g. with buffer-swap-text) and only keep in the
> "official" buffer the content-listing.

Yes, I agree completely.  Doing this sort of thing in a multibyte buffer
(with narrowing) is just asking for complications.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: arc-mode

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Yes.  However, I suspect this juggling is no longer need nowadays.  If
> you just use 'insert', does arc-mode break when you invoke the
> commands that end up using these calls?  This should be tried also in
> a locale whose codeset is not UTF-8.

The string bit in

(defun insert-unibyte (&rest args)
  "Like insert but don't make unibyte string and eight-bit char multibyte."
  (dolist (elt args)
    (if (integerp elt)
	(insert (if (< elt 128) elt (decode-char 'eight-bit elt)))
      (insert (string-to-multibyte elt)))))

is used two places; both for renaming (and they don't work).  But here's
the lzh version:

  (let ((name (concat newname (substring "\0\0\0\0\0\0\0\0\0\0\0\0\0"
					 (length newname))))
	(inhibit-read-only t))
    (save-restriction
      (save-excursion
	(widen)
	(goto-char (+ archive-proper-file-start (aref descr 4) 2))
	(delete-char 13)
	(insert-unibyte name)))))

If newname is ascii, then the string-to-multibyte thing isn't needed.
If newname is not ascii, then it fails, anyway:

(setq newname "f⚀o")
(setq name (concat newname (substring "\0\0\0\0\0\0\0\0\0\0\0\0\0"
(length newname))))
=> "f⚀o^@^@^@^@^@^@^@^@^@^@"

(insert-unibyte name)
=> f⚀o^@^@^@^@^@^@^@^@^@^@

And then do a

(multibyte-char-to-unibyte (char-after (point)))

on the die and get a

-1

which doesn't seem very useful, because it'll probably turn into an #ff
when the file is read back.

byte-after, on the other hand, will bug out:

Debugger entered--Lisp error: (error "Not an ASCII nor an 8-bit character: 9856")
  get-byte(2524)

so you're right -- we can't just replace the calls with get-byte, but
will have to add some error handling to be er feature compatible.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: arc-mode

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> (setq newname "f⚀o")

Sorry; that wasn't relevant:

(defun archive-rename-entry (newname)
  "Change the name associated with this entry in the archive file."
  (interactive "sNew name: ")

[...]

	  (funcall func
		   (encode-coding-string newname
					 archive-file-name-coding-system)
		   descr)

All the strings are encoded before being passed to the underlying
function, so that bit should be OK.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: arc-mode

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Sat, 18 May 2019 06:21:24 +0200
> 
> byte-after, on the other hand, will bug out:
> 
> Debugger entered--Lisp error: (error "Not an ASCII nor an 8-bit character: 9856")
>   get-byte(2524)
> 
> so you're right -- we can't just replace the calls with get-byte, but
> will have to add some error handling to be er feature compatible.

Yes, we need to wrap it in condition-case, at least for now.

Re: Towards a cleaner build: arc-mode

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> First of all, it defines defsubsts in the global namespace, but
>> (decode-char 'eight-bit 130) => 4194178
>> 
>> Hm...  Oh!  Is it working in a multibyte buffer, so it wants to insert
>> bytes that are in Emacs' "byte plane"?
>> 
>> (insert (decode-char 'eight-bit 130))
>> => \202
>> 
>> I guess.
>
> Yes.  However, I suspect this juggling is no longer need nowadays.  If
> you just use 'insert', does arc-mode break when you invoke the
> commands that end up using these calls?  This should be tried also in
> a locale whose codeset is not UTF-8.

Yes, if I just insert the bytes, things break.  In a zip file, I did a
chmod change to 0777 and in the new file, I got:

  -rwxrwxrwx         5  18-May-2019  05:27:06  text.txt

If I do the same, but replace

	(insert (if (< elt 128) elt (decode-char 'eight-bit elt)))

with just

        (insert elt)

I get:

  crws--S-wx         5  18-May-2019  05:27:06  ^@^@text.t

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: arc-mode

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Sat, 18 May 2019 06:36:25 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Yes.  However, I suspect this juggling is no longer need nowadays.  If
> > you just use 'insert', does arc-mode break when you invoke the
> > commands that end up using these calls?  This should be tried also in
> > a locale whose codeset is not UTF-8.
> 
> Yes, if I just insert the bytes, things break.  In a zip file, I did a
> chmod change to 0777 and in the new file, I got:
> 
>   -rwxrwxrwx         5  18-May-2019  05:27:06  text.txt
> 
> If I do the same, but replace
> 
> 	(insert (if (< elt 128) elt (decode-char 'eight-bit elt)))
> 
> with just
> 
>         (insert elt)
> 
> I get:
> 
>   crws--S-wx         5  18-May-2019  05:27:06  ^@^@text.t

Sorry, that's not what I meant.  (I guess I've misunderstood what you
were asking about, and thus my "juggling" was misinterpreted.)  I
meant to do this instead:

  (defun insert-unibyte (&rest args)
    "Like insert but don't make unibyte string and eight-bit char multibyte."
    (dolist (elt args)
      (if (integerp elt)
	  (insert (if (< elt 128) elt (decode-char 'eight-bit elt)))
	(insert elt))))

IOW, just lose the string-to-multibyte part, as I don't think it's
needed nowadays, since 'insert' already does the equivalent of
string-to-multibyte when needed.  The warning we want to get rid off
is about string-to-multibyte, not about decode-char, right?

Re: Towards a cleaner build: arc-mode

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Sorry, that's not what I meant.  (I guess I've misunderstood what you
> were asking about, and thus my "juggling" was misinterpreted.)  I
> meant to do this instead:
>
>   (defun insert-unibyte (&rest args)
>     "Like insert but don't make unibyte string and eight-bit char multibyte."
>     (dolist (elt args)
>       (if (integerp elt)
> 	  (insert (if (< elt 128) elt (decode-char 'eight-bit elt)))
> 	(insert elt))))
>
> IOW, just lose the string-to-multibyte part, as I don't think it's
> needed nowadays, since 'insert' already does the equivalent of
> string-to-multibyte when needed.  The warning we want to get rid off
> is about string-to-multibyte, not about decode-char, right?

Ah, right, yes.

But that's unfortunately the part I'm not able to test since the
commands that use that path don't actually work...

But I think you're right -- the two functions that insert a string
insert only encoded strings (with some binary stuff afterwards), so I
think that change should be safe.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: arc-mode

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> But that's unfortunately the part I'm not able to test since the
> commands that use that path don't actually work...

I was able to test your suggestion here with .arc files (which support
file name editing), and I verified that the edited files were identical
before and after the change.  

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: arc-mode

[Eli Zaretskii]

On May 19, 2019 5:36:58 PM GMT+01:00, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > But that's unfortunately the part I'm not able to test since the
> > commands that use that path don't actually work...
> 
> I was able to test your suggestion here with .arc files (which support
> file name editing), and I verified that the edited files were
> identical
> before and after the change.  

Then I think we are good, thanks.

Re: Towards a cleaner build: ehelp

[Lars Ingebrigtsen]

In electric-help-execute-extended:
ehelp.el:358:10:Warning: ‘execute-extended-command’ is for interactive use
    only; use ‘command-execute’ instead.

This is the function definition:

;; This is to be bound to M-x in ehelp mode. Retains ehelp buffer and then
;; continues with execute-extended-command.
(defun electric-help-execute-extended (_prefixarg)
  (interactive "p")
  (setq electric-help-form-to-execute
        (lambda () (execute-extended-command nil)))
  (electric-help-retain))

I think ehelp is perhaps doing exactly the right thing here?  It's
creating a variation of `M-x', so calling `execute-extended-command' is
the right thing to do here?

Slap a with-no-warnings around it?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: ehelp

[Stefan Monnier]

> ;; This is to be bound to M-x in ehelp mode. Retains ehelp buffer and then
> ;; continues with execute-extended-command.
> (defun electric-help-execute-extended (_prefixarg)
>   (interactive "p")
>   (setq electric-help-form-to-execute
>         (lambda () (execute-extended-command nil)))
>   (electric-help-retain))
>
> I think ehelp is perhaps doing exactly the right thing here?  It's
> creating a variation of `M-x', so calling `execute-extended-command' is
> the right thing to do here?
>
> Slap a with-no-warnings around it?

Maybe even better: call it with call-interactively?


        Stefan

Re: Towards a cleaner build: ehelp

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> ;; This is to be bound to M-x in ehelp mode. Retains ehelp buffer and then
>> ;; continues with execute-extended-command.
>> (defun electric-help-execute-extended (_prefixarg)
>>   (interactive "p")
>>   (setq electric-help-form-to-execute
>>         (lambda () (execute-extended-command nil)))
>>   (electric-help-retain))

[...]

> Maybe even better: call it with call-interactively?

Hm...  but would that be the same?  With call-interactively, PREFIXARG
won't be nil, will it?  I don't know whether that matters, though...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: hexl

[Lars Ingebrigtsen]

In hexl-insert-multibyte-char:
hexl.el:881:29:Warning: ‘string-as-unibyte’ is an obsolete function (as of
    26.1); use ‘encode-coding-string’.

This is the code...

	   (let ((encoded (encode-coding-char ch coding))
		 (internal (string-as-unibyte (char-to-string ch)))
		 internal-hex)
	     ;; If encode-coding-char returns nil, it means our character
	     ;; cannot be safely encoded with buffer-file-coding-system.
	     ;; In that case, we offer to insert the internal representation
	     ;; of that character, byte by byte.

And here we really, really want the internal byte representation of the
multibyte string, so I think the code is correct and just needs a
with-no-warnings?  I mean,

  (with-suppressed-warnings ((obsolete string-as-unibyte))
    (string-as-unibyte (char-to-string ch)))

:-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: hexl

[Noam Postavsky]

On Fri, 17 May 2019 at 08:45, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
> In hexl-insert-multibyte-char:
> hexl.el:881:29:Warning: ‘string-as-unibyte’ is an obsolete function (as of
>     26.1); use ‘encode-coding-string’.
>
> This is the code...
>
>            (let ((encoded (encode-coding-char ch coding))
>                  (internal (string-as-unibyte (char-to-string ch)))
>                  internal-hex)
>              ;; If encode-coding-char returns nil, it means our character
>              ;; cannot be safely encoded with buffer-file-coding-system.
>              ;; In that case, we offer to insert the internal representation
>              ;; of that character, byte by byte.
>
> And here we really, really want the internal byte representation of the
> multibyte string,

Shouldn't we use (encode-coding-char ch 'utf-8-emacs) instead?

Re: Towards a cleaner build: hexl

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 17 May 2019 14:45:23 +0200
> 
> 	   (let ((encoded (encode-coding-char ch coding))
> 		 (internal (string-as-unibyte (char-to-string ch)))
> 		 internal-hex)
> 	     ;; If encode-coding-char returns nil, it means our character
> 	     ;; cannot be safely encoded with buffer-file-coding-system.
> 	     ;; In that case, we offer to insert the internal representation
> 	     ;; of that character, byte by byte.
> 
> And here we really, really want the internal byte representation of the
> multibyte string, so I think the code is correct and just needs a
> with-no-warnings?  I mean,
> 
>   (with-suppressed-warnings ((obsolete string-as-unibyte))
>     (string-as-unibyte (char-to-string ch)))

Howe about this instead:

     (let ((encoded (encode-coding-char ch coding))
	   (internal (char-to-string ch))
	   internal-hex)
       ;; If encode-coding-char returns nil, it means our character
       ;; cannot be safely encoded with buffer-file-coding-system.
       ;; In that case, we offer to insert the internal representation
       ;; of that character, byte by byte.
       (when (null encoded)
	 (setq internal (encode-coding-string internal 'utf-8-emacs)
	       internal-hex
	        (mapconcat (function (lambda (c) (format "%x" c)))
			   internal " "))))

Does this work?

Re: Towards a cleaner build: hexl

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Howe about this instead:
>
>      (let ((encoded (encode-coding-char ch coding))
> 	   (internal (char-to-string ch))
> 	   internal-hex)
>        ;; If encode-coding-char returns nil, it means our character
>        ;; cannot be safely encoded with buffer-file-coding-system.
>        ;; In that case, we offer to insert the internal representation
>        ;; of that character, byte by byte.
>        (when (null encoded)
> 	 (setq internal (encode-coding-string internal 'utf-8-emacs)
> 	       internal-hex
> 	        (mapconcat (function (lambda (c) (format "%x" c)))
> 			   internal " "))))
>
> Does this work?

Yes, it works for me if in my test case where char is ⚀ and the coding
system is iso-8859-1.  I'll make the change and push.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: hexl

[Stefan Monnier]

> 	   (let ((encoded (encode-coding-char ch coding))
> 		 (internal (string-as-unibyte (char-to-string ch)))
> 		 internal-hex)
> 	     ;; If encode-coding-char returns nil, it means our character
> 	     ;; cannot be safely encoded with buffer-file-coding-system.
> 	     ;; In that case, we offer to insert the internal representation
> 	     ;; of that character, byte by byte.
>
> And here we really, really want the internal byte representation of the
> multibyte string, so I think the code is correct and just needs a
> with-no-warnings?  I mean,
>
>   (with-suppressed-warnings ((obsolete string-as-unibyte))
>     (string-as-unibyte (char-to-string ch)))

FWIW, this fallback behavior is a misfeature.
We should rather signal an error.

But I also consider hexl-mode as obsoleted by nhexl-mode, so my opinion
is probably not terribly relevant,


        Stefan

Re: Towards a cleaner build: hexl

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> FWIW, this fallback behavior is a misfeature.
> We should rather signal an error.

Yeah, it doesn't seem particularly useful...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: custom

[Lars Ingebrigtsen]

In custom-variable-value-create:
cus-edit.el:2584:24:Warning: ‘custom-show’ is an obsolete function (as of
    24.1); this widget type is no longer supported.

It's this code:

    ;; If we don't know the state, see if we need to edit it in lisp form.
    (unless state
      (setq state (if (custom-show type value) 'unknown 'hidden)))

24.1 is a long time ago, so changing that to

    ;; If we don't know the state, see if we need to edit it in lisp form.
    (unless state
      (setq state 'unknown))

is perhaps the right solution?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: frameset

[Lars Ingebrigtsen]

In frameset-to-register:
frameset.el:1403:18:Warning: ‘registerv-make’ is an obsolete function (as of
    27.1); Use your own type with methods on
    register-val-(insert|describe|jump-to)

It's this code:

  (set-register register
		(registerv-make
		 (vector (frameset-save nil
					:app 'register
					:filters frameset-session-filter-alist)
			 ;; frameset-save does not include the value of point
			 ;; in the current buffer, so record that separately.
			 (frameset-frame-id nil)
			 (point-marker))
		 :print-func #'frameset--print-register
		 :jump-func #'frameset--jump-to-register)))

I've never used register-val-*, so I have no idea what to do here...
anybody?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: frameset

[Stefan Monnier]

> In frameset-to-register:
> frameset.el:1403:18:Warning: ‘registerv-make’ is an obsolete function (as of
>     27.1); Use your own type with methods on
>     register-val-(insert|describe|jump-to)
>
> It's this code:
>
>   (set-register register
> 		(registerv-make
> 		 (vector (frameset-save nil
> 					:app 'register
> 					:filters frameset-session-filter-alist)
> 			 ;; frameset-save does not include the value of point
> 			 ;; in the current buffer, so record that separately.
> 			 (frameset-frame-id nil)
> 			 (point-marker))
> 		 :print-func #'frameset--print-register
> 		 :jump-func #'frameset--jump-to-register)))

registerv-make faked OO-style programming by having objects that carry
their methods.

> I've never used register-val-*, so I have no idea what to do here...
> anybody?

The new style uses CLOS-style OO-programming instead.  I.e. the object
you put into the register should have its own distinctive type
(i.e. not `registerv`) and you define the print/jump/.. methods with
`cl-defmethod`.

So, here you'd want to use a cl-defstruct to replace the `(vector ...)`
object and then use dispatch on this defstruct's type when
cl-defmethod'ing the register-val-print/jump/... functions.

Does that make sense?


        Stefan

Re: Towards a cleaner build: frameset

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I've never used register-val-*, so I have no idea what to do here...
>> anybody?
>
> The new style uses CLOS-style OO-programming instead.  I.e. the object
> you put into the register should have its own distinctive type
> (i.e. not `registerv`) and you define the print/jump/.. methods with
> `cl-defmethod`.
>
> So, here you'd want to use a cl-defstruct to replace the `(vector ...)`
> object and then use dispatch on this defstruct's type when
> cl-defmethod'ing the register-val-print/jump/... functions.
>
> Does that make sense?

It does indeed.

I tried to see whether there was anything that had already gone through
this transformation so that I could just cargo cult it, but apparently
not, and I don't use any of the packages that needs this transformation,
so I think I'll just leave it to somebody else to do the rewrite and
testing.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: frameset

[Stefan Monnier]

> I tried to see whether there was anything that had already gone through
> this transformation

Je crois que le plus proche serait le cas de `registerv` lui-même.

> so that I could just cargo cult it, but apparently not, and I don't
> use any of the packages that needs this transformation, so I think
> I'll just leave it to somebody else to do the rewrite and testing.

C'est maintenant fait pour frameset.  Il reste kmacro et senator.


        Stefan

Re: Towards a cleaner build: frameset

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> so that I could just cargo cult it, but apparently not, and I don't
>> use any of the packages that needs this transformation, so I think
>> I'll just leave it to somebody else to do the rewrite and testing.
>
> C'est maintenant fait pour frameset.  Il reste kmacro et senator.

:-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: frameset

[Stefan Monnier]

>>> so that I could just cargo cult it, but apparently not, and I don't
>>> use any of the packages that needs this transformation, so I think
>>> I'll just leave it to somebody else to do the rewrite and testing.
>> C'est maintenant fait pour frameset.  Il reste kmacro et senator.

Pardon my French!


        Stef "literally for once"

Re: Towards a cleaner build: epa-file

[Lars Ingebrigtsen]

In epa-file-decode-and-insert:
epa-file.el:109:22:Warning: ‘string-to-multibyte’ is an obsolete function (as
    of 26.1); use ‘decode-coding-string’.

Here's the code:

(defun epa-file-decode-and-insert (string file visit beg end replace)
  (if (fboundp 'decode-coding-inserted-region)
      (save-restriction
	(narrow-to-region (point) (point))
	(insert (if enable-multibyte-characters
		    (string-to-multibyte string)
		  string))
	(decode-coding-inserted-region
	 (point-min) (point-max)
	 (substring file 0 (string-match epa-file-name-regexp file))
	 visit beg end replace))
    (insert (epa-file--decode-coding-string string (or coding-system-for-read
						       'undecided)))))

Isn't that check for `enable-multibyte-characters' and
call to `string-to-multibyte' completely superfluous?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Towards a cleaner build: epa-file

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sat, 18 May 2019 10:42:28 +0200
> 
> (defun epa-file-decode-and-insert (string file visit beg end replace)
>   (if (fboundp 'decode-coding-inserted-region)
>       (save-restriction
> 	(narrow-to-region (point) (point))
> 	(insert (if enable-multibyte-characters
> 		    (string-to-multibyte string)
> 		  string))
> 	(decode-coding-inserted-region
> 	 (point-min) (point-max)
> 	 (substring file 0 (string-match epa-file-name-regexp file))
> 	 visit beg end replace))
>     (insert (epa-file--decode-coding-string string (or coding-system-for-read
> 						       'undecided)))))
> 
> Isn't that check for `enable-multibyte-characters' and
> call to `string-to-multibyte' completely superfluous?

Yes, I think 'insert' already does that internally.

Re: Towards a cleaner build: epa-file

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Isn't that check for `enable-multibyte-characters' and
>> call to `string-to-multibyte' completely superfluous?
>
> Yes, I think 'insert' already does that internally.

OK; I'll remove that bit.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no