url/url.el - Duplicate requires

url/url.el - Duplicate requires

[Cheng Gao]

Line 44-46:
,----
| (eval-when-compile
|   (require 'mm-decode)
|   (require 'mm-view))
`----
But line 237-241:
,----
| (defun url-mm-url (url)
|   "Retrieve URL and pass to the appropriate viewing application."
|   (require 'mm-decode)
|   (require 'mm-view)
|   (url-retrieve url 'url-mm-callback nil))
`----
mm-decode and mm-view are required again.

Should the requires in function url-mm-url be removed?

Re: url/url.el - Duplicate requires

[David Kastrup]

Cheng Gao <chenggao@gmail.com> writes:

> Line 44-46:
> ,----
> | (eval-when-compile
> |   (require 'mm-decode)
> |   (require 'mm-view))
> `----
> But line 237-241:
> ,----
> | (defun url-mm-url (url)
> |   "Retrieve URL and pass to the appropriate viewing application."
> |   (require 'mm-decode)
> |   (require 'mm-view)
> |   (url-retrieve url 'url-mm-callback nil))
> `----
> mm-decode and mm-view are required again.
>
> Should the requires in function url-mm-url be removed?

require at run-time is a bad idea, and constructs like above with
eval-when-compile tend to create byte compiler warnings.  I think the
correct way to deal with this would be something like

(autoload 'url-retrieve 'mm-decode)
(defun url-mm-url (url)
  (url-retrieve url ...

That way, the byte compiler is satisfied, and no runtime performance
hit is scored by unneeded requires.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: url/url.el - Duplicate requires

[Stefan Monnier]

> Line 44-46:
> ,----
> | (eval-when-compile
> |   (require 'mm-decode)
> |   (require 'mm-view))
> `----
> But line 237-241:
> ,----
> | (defun url-mm-url (url)
> |   "Retrieve URL and pass to the appropriate viewing application."
> |   (require 'mm-decode)
> |   (require 'mm-view)
> |   (url-retrieve url 'url-mm-callback nil))
> `----
> mm-decode and mm-view are required again.

> Should the requires in function url-mm-url be removed?

The requires aren't done at the same time so they are "unrelated".
The first ones above are just there to quieten the byte-compiler and to
define some macros (e.g. mm-handle-undisplayer) used during
byte-compilation.  The second are because url-mm-callback actually uses
functions from those libs at run time.

So all in all, the current code looks OK.  I'd be tempted to move the last
two require from url-mm-url (where the functions they define aren't
actually used) to url-mm-callback (where they are used), or to define
autoloads, as suggested by David.  But I suspect there's a good reason why
they're in url-mm-url, probably having to do with the fact that
url-mm-callback is usually called from a process sentinel or process filter,
and loading mm-decode or mm-view at that point may lead to problems (maybe
some global vars are let-bound at that point and interact poorly with the
loading of mm-decode or mm-view).  None of the historical data (such as the
ChangeLog or the CVS files in the older URL subversions package) allows me
to confirm since this code predates the URL package (it probably goes back
to the time when URL was part of W3).

I.e. if I were you, I'd add a comment explaining something like the above as
a justification for why we didn't change the code.


        Stefan