Towards a cleaner build

classic Classic list List threaded Threaded
279 messages Options
1234 ... 14
Reply | Threaded
Open this post in threaded view
|

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


Reply | Threaded
Open this post in threaded view
|

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


Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Lars Ingebrigtsen
In reply to this post by 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


Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Lars Ingebrigtsen
In reply to this post by 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


Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Lars Ingebrigtsen
In reply to this post by 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


Reply | Threaded
Open this post in threaded view
|

RE: Towards a cleaner build

Drew Adams
In reply to this post by Lars Ingebrigtsen
> 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."

Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Lars Ingebrigtsen
In reply to this post by 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


Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Lars Ingebrigtsen
In reply to this post by 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


Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Stefan Monnier
In reply to this post by Lars Ingebrigtsen
> 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


Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Stefan Monnier
In reply to this post by Lars Ingebrigtsen
Lars Ingebrigtsen <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Lars Ingebrigtsen
In reply to this post by Stefan Monnier
Stefan Monnier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Lars Ingebrigtsen
In reply to this post by Stefan Monnier
Stefan Monnier <[hidden email]> writes:

> Lars Ingebrigtsen <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Eli Zaretskii
In reply to this post by Lars Ingebrigtsen
> From: Lars Ingebrigtsen <[hidden email]>
> 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?

Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Eli Zaretskii
In reply to this post by Lars Ingebrigtsen
> From: Lars Ingebrigtsen <[hidden email]>
> Date: Fri, 17 May 2019 06:29:51 +0200
> Cc: [hidden email]
>
> >     (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.

Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Lars Ingebrigtsen
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Lars Ingebrigtsen
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Lars Ingebrigtsen
Lars Ingebrigtsen <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Eli Zaretskii
In reply to this post by Lars Ingebrigtsen
> From: Lars Ingebrigtsen <[hidden email]>
> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Eli Zaretskii
In reply to this post by Lars Ingebrigtsen
> From: Lars Ingebrigtsen <[hidden email]>
> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Towards a cleaner build

Eli Zaretskii
In reply to this post by Lars Ingebrigtsen
> From: Lars Ingebrigtsen <[hidden email]>
> Cc: [hidden email]
> Date: Fri, 17 May 2019 07:52:13 +0200
>
> Eli Zaretskii <[hidden email]> 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?

1234 ... 14