Changes in calendar/time-date.el (was: CVS update of gnus/lisp (4 files))

Changes in calendar/time-date.el (was: CVS update of gnus/lisp (4 files))

[Reiner Steib]

On Wed, Mar 30 2005, Katsumi Yamaoka wrote:

> There's no ChangeLog for the recent changes of time-date.el and
> I don't know who did it for what purpose, 

[ I'm adding emacs-devel. ]
In Emacs, `time-date.el' is located in the calendar subdirectory.
Probably Miles' script cannot fetch the ChangeLog automatically.

I think this is the relevant entry:

,----[ lisp/ChangeLog ]
| 2005-03-23  Lute Kamstra  <lute@gnu.org>
| [...]
| 	* calendar/time-date.el: Add comment on time value formats.
| 	Don't require parse-time.
| 	(with-decoded-time-value): New macro.
| 	(encode-time-value): New function.
| 	(time-to-seconds, time-less-p, time-subtract, time-add): Use them.
| 	(days-to-time): Return a valid time value when arg is huge.
| 	(time-since): Use time-subtract.
| 	(time-to-number-of-days): Use time-to-seconds.
`----

> but at least the byte compiler complains to me about some missing
> Lisp objects.  What should we do for that?

Probably we need to add (require 'parse-time) to nnimap.el,
nnultimate.el and pop3.el.

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Changes in calendar/time-date.el (was: CVS update of gnus/lisp (4 files))

[Miles Bader]

On Mar 31, 2005 8:27 PM, Reiner Steib <reinersteib+gmane@imap.cc> wrote:
> > There's no ChangeLog for the recent changes of time-date.el and
> > I don't know who did it for what purpose,
> 
> [ I'm adding emacs-devel. ]
> In Emacs, `time-date.el' is located in the calendar subdirectory.
> Probably Miles' script cannot fetch the ChangeLog automatically.

Yeah; I should have caught it though. :-/  Sorry about that.

-Miles
-- 
Do not taunt Happy Fun Ball.

Re: Changes in calendar/time-date.el

[Lute Kamstra]

Reiner Steib <reinersteib+gmane@imap.cc> writes:

> On Wed, Mar 30 2005, Katsumi Yamaoka wrote:
>
>> There's no ChangeLog for the recent changes of time-date.el and
>> I don't know who did it for what purpose, 
>
> [ I'm adding emacs-devel. ]
> In Emacs, `time-date.el' is located in the calendar subdirectory.
> Probably Miles' script cannot fetch the ChangeLog automatically.
>
> I think this is the relevant entry:
>
> ,----[ lisp/ChangeLog ]
> | 2005-03-23  Lute Kamstra  <lute@gnu.org>
> | [...]
> | 	* calendar/time-date.el: Add comment on time value formats.
> | 	Don't require parse-time.
> | 	(with-decoded-time-value): New macro.
> | 	(encode-time-value): New function.
> | 	(time-to-seconds, time-less-p, time-subtract, time-add): Use them.
> | 	(days-to-time): Return a valid time value when arg is huge.
> | 	(time-since): Use time-subtract.
> | 	(time-to-number-of-days): Use time-to-seconds.
> `----
>
>> but at least the byte compiler complains to me about some missing
>> Lisp objects.  What should we do for that?
>
> Probably we need to add (require 'parse-time) to nnimap.el,
> nnultimate.el and pop3.el.

I gather from this message that time-date's home is actually Gnus'
CVS?  I wasn't aware of that.  In Emacs' CVS it's in lisp/calendar and
not in lisp/gnus, so ChangeLog entries go to lisp/ChangeLog and not
lisp/gnus/ChangeLog.  This should probably be dealt with manually when
synchronizing Emacs' CVS and Gnus' CVS.

I removed (require 'parse-time) from time-date because it uses just
parse-time-string, which is autoloaded.  Do I understand correctly my
change uncovered some bugs in nnimap.el, nnultimate.el and pop3.el?  I
already noticed (and fixed) this for message.el.  I don't use
nnimap.el, nnultimate.el or pop3.el so I didn't catch those.  I'm
sorry for that.

Lute.

Re: Changes in calendar/time-date.el

[Miles Bader]

Lute Kamstra <Lute.Kamstra.lists@xs4all.nl> writes:
> I gather from this message that time-date's home is actually Gnus'
> CVS?  I wasn't aware of that.

It does seem to have been originally written by the Gnus authors, but
who knows where its "home" is these days; it exists in both CVS trees
anyway.

> In Emacs' CVS it's in lisp/calendar and not in lisp/gnus, so ChangeLog
> entries go to lisp/ChangeLog and not lisp/gnus/ChangeLog.  This should
> probably be dealt with manually when synchronizing Emacs' CVS and
> Gnus' CVS.

Right.

-Miles
-- 
Run away!  Run away!

Re: Changes in calendar/time-date.el

[Richard Stallman]

    I gather from this message that time-date's home is actually Gnus'
    CVS?

That may have been true in the past, but nowadays time-date.el is an
ordinary file of Emacs, maintained the same way as most Emacs files.
As far as we are concerned it is not a part of Gnus.  

Re: Changes in calendar/time-date.el

[Reiner Steib]

On Thu, Mar 31 2005, Lute Kamstra wrote:

> Reiner Steib <reinersteib+gmane@imap.cc> writes:
>> On Wed, Mar 30 2005, Katsumi Yamaoka wrote:
>>> There's no ChangeLog for the recent changes of time-date.el and
>>> I don't know who did it for what purpose, 
[...]
>> ,----[ lisp/ChangeLog ]
>> | 2005-03-23  Lute Kamstra  <lute@gnu.org>
>> | [...]
>> | 	* calendar/time-date.el: Add comment on time value formats.
>> | 	Don't require parse-time.
>> | 	(with-decoded-time-value): New macro.
>> | 	(encode-time-value): New function.
>> | 	(time-to-seconds, time-less-p, time-subtract, time-add): Use them.
>> | 	(days-to-time): Return a valid time value when arg is huge.
>> | 	(time-since): Use time-subtract.
>> | 	(time-to-number-of-days): Use time-to-seconds.
>> `----

I have added a ChangeLog in Gnus lisp/ChangeLog.

>>> but at least the byte compiler complains to me about some missing
>>> Lisp objects.  What should we do for that?
>>
>> Probably we need to add (require 'parse-time) to nnimap.el,
>> nnultimate.el and pop3.el.
[...]
> I removed (require 'parse-time) from time-date because it uses just
> parse-time-string, which is autoloaded.  Do I understand correctly my
> change uncovered some bugs in nnimap.el, nnultimate.el and pop3.el?  I
> already noticed (and fixed) this for message.el.  

Wouldn't it be better to put (require 'parse-time) at the beginning of
the file instead of inside `message-make-date'?

> I don't use nnimap.el, nnultimate.el or pop3.el so I didn't catch
> those.  I'm sorry for that.

I added (require 'parse-time) at the beginning of those files in Gnus
stable branch.

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Changes in calendar/time-date.el

[Lute Kamstra]

Reiner Steib <reinersteib+gmane@imap.cc> writes:

>> I removed (require 'parse-time) from time-date because it uses just
>> parse-time-string, which is autoloaded.  Do I understand correctly my
>> change uncovered some bugs in nnimap.el, nnultimate.el and pop3.el?  I
>> already noticed (and fixed) this for message.el.  
>
> Wouldn't it be better to put (require 'parse-time) at the beginning of
> the file instead of inside `message-make-date'?

message-make-date is the only function in message.el that uses
parse-time, so it's best to put the require there.  If you change
message-make-date so that it no longer uses parse-time, you will
probably notice the require and remove it.  Had you put require at the
beginning of the file, you would most likely forget this.  Putting a
require at the beginning of a file is useful if the file uses a
particular feature a lot.

Lute.

Re: Changes in calendar/time-date.el

[Reiner Steib]

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

On Mon, Apr 04 2005, Lute Kamstra wrote:

> Reiner Steib <reinersteib+gmane@imap.cc> writes:
>> Wouldn't it be better to put (require 'parse-time) at the beginning of
>> the file instead of inside `message-make-date'?
>
> message-make-date is the only function in message.el that uses
> parse-time, so it's best to put the require there.  If you change
> message-make-date so that it no longer uses parse-time, you will
> probably notice the require and remove it.  Had you put require at the
> beginning of the file, you would most likely forget this.  Putting a
> require at the beginning of a file is useful if the file uses a
> particular feature a lot.

Okay, I agree.  Putting it at the beginning of a file additionally
avoids some "reference to free variable" warnings.  The following
patch[1] would avoid them too.  (autoload 'parse-time-string ...)
should be there for the standalone Gnus which is supposed to work with
older Emacs versions.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: parse-time-loads.patch --]
[-- Type: text/x-patch, Size: 2978 bytes --]

Index: message.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/gnus/message.el,v
retrieving revision 1.75
diff -u -r1.75 message.el
--- message.el	30 Mar 2005 08:14:32 -0000	1.75
+++ message.el	4 Apr 2005 11:36:12 -0000
@@ -32,6 +32,8 @@
 
 (eval-when-compile
   (require 'cl)
+  (defvar parse-time-weekdays) ;; parse-time is required where necessary
+  (defvar parse-time-months)
   (defvar gnus-message-group-art)
   (defvar gnus-list-identifiers)) ; gnus-sum is required where necessary
 (require 'canlock)
Index: nnimap.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/gnus/nnimap.el,v
retrieving revision 1.23
diff -u -r1.23 nnimap.el
--- nnimap.el	18 Mar 2005 02:49:57 -0000	1.23
+++ nnimap.el	4 Apr 2005 11:36:12 -0000
@@ -69,6 +69,8 @@
 (require 'gnus-range)
 (require 'gnus-start)
 (require 'gnus-int)
+ ;; Only for `parse-time-months' in `nnimap-date-days-ago':
+(require 'parse-time)
 
 (eval-when-compile (require 'cl))
 
Index: nnultimate.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/gnus/nnultimate.el,v
retrieving revision 1.5
diff -u -r1.5 nnultimate.el
--- nnultimate.el	4 Sep 2004 13:13:44 -0000	1.5
+++ nnultimate.el	4 Apr 2005 11:36:12 -0000
@@ -39,6 +39,7 @@
 (require 'mm-util)
 (require 'mm-url)
 (require 'nnweb)
+(require 'parse-time)
 (autoload 'w3-parse-buffer "w3-parse")
 
 (nnoo-declare nnultimate)
Index: gnus-art.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/gnus/gnus-art.el,v
retrieving revision 1.65
diff -u -r1.65 gnus-art.el
--- gnus-art.el	18 Mar 2005 02:49:57 -0000	1.65
+++ gnus-art.el	4 Apr 2005 11:36:13 -0000
@@ -46,6 +46,7 @@
 (autoload 'gnus-msg-mail "gnus-msg" nil t)
 (autoload 'gnus-button-mailto "gnus-msg")
 (autoload 'gnus-button-reply "gnus-msg" nil t)
+(autoload 'parse-time-string "parse-time" nil nil)
 
 (defgroup gnus-article nil
   "Article display."
Index: gnus-demon.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/gnus/gnus-demon.el,v
retrieving revision 1.9
diff -u -r1.9 gnus-demon.el
--- gnus-demon.el	4 Sep 2004 13:13:43 -0000	1.9
+++ gnus-demon.el	4 Apr 2005 11:36:13 -0000
@@ -40,6 +40,8 @@
       (require 'itimer)
     (require 'timer)))
 
+(autoload 'parse-time-string "parse-time" nil nil)
+
 (defgroup gnus-demon nil
   "Demonic behaviour."
   :group 'gnus)
Index: gnus-delay.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/gnus/gnus-delay.el,v
retrieving revision 1.5
diff -u -r1.5 gnus-delay.el
--- gnus-delay.el	9 Feb 2005 15:50:39 -0000	1.5
+++ gnus-delay.el	4 Apr 2005 11:36:13 -0000
@@ -37,6 +37,7 @@
 
 (require 'nndraft)
 (require 'gnus-draft)
+(autoload 'parse-time-string "parse-time" nil nil)
 
 ;;;###autoload
 (defgroup gnus-delay nil

[-- Attachment #3: Type: text/plain, Size: 171 bytes --]


Does it look correct?

Bye, Reiner.

[1] The patch is for Emacs' CVS.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Changes in calendar/time-date.el

[Lute Kamstra]

Reiner Steib <reinersteib+gmane@imap.cc> writes:

>>> Wouldn't it be better to put (require 'parse-time) at the beginning of
>>> the file instead of inside `message-make-date'?
>>
>> message-make-date is the only function in message.el that uses
>> parse-time, so it's best to put the require there.  If you change
>> message-make-date so that it no longer uses parse-time, you will
>> probably notice the require and remove it.  Had you put require at the
>> beginning of the file, you would most likely forget this.  Putting a
>> require at the beginning of a file is useful if the file uses a
>> particular feature a lot.

I forgot to mention that putting the require inside the defun has the
advantage that the feature gets loaded at the moment it it needed.  If
the function is not called, the feature isn't loaded.

> Okay, I agree.  Putting it at the beginning of a file additionally
> avoids some "reference to free variable" warnings.  The following
> patch[1] would avoid them too.  (autoload 'parse-time-string ...)
> should be there for the standalone Gnus which is supposed to work with
> older Emacs versions.
>
> Index: message.el
> ===================================================================
> RCS file: /cvsroot/emacs/emacs/lisp/gnus/message.el,v
> retrieving revision 1.75
> diff -u -r1.75 message.el
> --- message.el	30 Mar 2005 08:14:32 -0000	1.75
> +++ message.el	4 Apr 2005 11:36:12 -0000
> @@ -32,6 +32,8 @@
>  
>  (eval-when-compile
>    (require 'cl)
> +  (defvar parse-time-weekdays) ;; parse-time is required where necessary
> +  (defvar parse-time-months)
>    (defvar gnus-message-group-art)
>    (defvar gnus-list-identifiers)) ; gnus-sum is required where necessary
>  (require 'canlock)

Can't you move this closer to the definition of message-make-date?
It's only necessary to suppress compiler warnings for that function.
Alternatively, you can use (with-no-warnings ...) around references to
these variables.

> Index: nnimap.el
> ===================================================================
> RCS file: /cvsroot/emacs/emacs/lisp/gnus/nnimap.el,v
> retrieving revision 1.23
> diff -u -r1.23 nnimap.el
> --- nnimap.el	18 Mar 2005 02:49:57 -0000	1.23
> +++ nnimap.el	4 Apr 2005 11:36:12 -0000
> @@ -69,6 +69,8 @@
>  (require 'gnus-range)
>  (require 'gnus-start)
>  (require 'gnus-int)
> + ;; Only for `parse-time-months' in `nnimap-date-days-ago':
> +(require 'parse-time)
>  
>  (eval-when-compile (require 'cl))
>  

Why not put the require in the body of nnimap-date-days-ago?

> Index: nnultimate.el
> ===================================================================
> RCS file: /cvsroot/emacs/emacs/lisp/gnus/nnultimate.el,v
> retrieving revision 1.5
> diff -u -r1.5 nnultimate.el
> --- nnultimate.el	4 Sep 2004 13:13:44 -0000	1.5
> +++ nnultimate.el	4 Apr 2005 11:36:12 -0000
> @@ -39,6 +39,7 @@
>  (require 'mm-util)
>  (require 'mm-url)
>  (require 'nnweb)
> +(require 'parse-time)
>  (autoload 'w3-parse-buffer "w3-parse")
>  
>  (nnoo-declare nnultimate)
> Index: gnus-art.el
> ===================================================================
> RCS file: /cvsroot/emacs/emacs/lisp/gnus/gnus-art.el,v
> retrieving revision 1.65
> diff -u -r1.65 gnus-art.el
> --- gnus-art.el	18 Mar 2005 02:49:57 -0000	1.65
> +++ gnus-art.el	4 Apr 2005 11:36:13 -0000
> @@ -46,6 +46,7 @@
>  (autoload 'gnus-msg-mail "gnus-msg" nil t)
>  (autoload 'gnus-button-mailto "gnus-msg")
>  (autoload 'gnus-button-reply "gnus-msg" nil t)
> +(autoload 'parse-time-string "parse-time" nil nil)
>  
>  (defgroup gnus-article nil
>    "Article display."
> Index: gnus-demon.el
> ===================================================================
> RCS file: /cvsroot/emacs/emacs/lisp/gnus/gnus-demon.el,v
> retrieving revision 1.9
> diff -u -r1.9 gnus-demon.el
> --- gnus-demon.el	4 Sep 2004 13:13:43 -0000	1.9
> +++ gnus-demon.el	4 Apr 2005 11:36:13 -0000
> @@ -40,6 +40,8 @@
>        (require 'itimer)
>      (require 'timer)))
>  
> +(autoload 'parse-time-string "parse-time" nil nil)
> +
>  (defgroup gnus-demon nil
>    "Demonic behaviour."
>    :group 'gnus)
> Index: gnus-delay.el
> ===================================================================
> RCS file: /cvsroot/emacs/emacs/lisp/gnus/gnus-delay.el,v
> retrieving revision 1.5
> diff -u -r1.5 gnus-delay.el
> --- gnus-delay.el	9 Feb 2005 15:50:39 -0000	1.5
> +++ gnus-delay.el	4 Apr 2005 11:36:13 -0000
> @@ -37,6 +37,7 @@
>  
>  (require 'nndraft)
>  (require 'gnus-draft)
> +(autoload 'parse-time-string "parse-time" nil nil)
>  
>  ;;;###autoload
>  (defgroup gnus-delay nil
>
> Does it look correct?

The rest looks fine to me.

Lute.

Re: Changes in calendar/time-date.el

[Reiner Steib]

On Mon, Apr 04 2005, Lute Kamstra wrote:

> Reiner Steib <reinersteib+gmane@imap.cc> writes:
>> --- message.el	30 Mar 2005 08:14:32 -0000	1.75
>> +++ message.el	4 Apr 2005 11:36:12 -0000
>> @@ -32,6 +32,8 @@
>>  
>>  (eval-when-compile
>>    (require 'cl)
>> +  (defvar parse-time-weekdays) ;; parse-time is required where necessary
>> +  (defvar parse-time-months)
>>    (defvar gnus-message-group-art)
>>    (defvar gnus-list-identifiers)) ; gnus-sum is required where necessary
>>  (require 'canlock)
>
> Can't you move this closer to the definition of message-make-date?
> It's only necessary to suppress compiler warnings for that function.

I put it inside the defun.

> Alternatively, you can use (with-no-warnings ...) around references to
> these variables.

`with-no-warnings' isn't available in Emacs 21.

>> --- nnimap.el	18 Mar 2005 02:49:57 -0000	1.23
>> +++ nnimap.el	4 Apr 2005 11:36:12 -0000
>> @@ -69,6 +69,8 @@
>>  (require 'gnus-range)
>>  (require 'gnus-start)
>>  (require 'gnus-int)
>> + ;; Only for `parse-time-months' in `nnimap-date-days-ago':
>> +(require 'parse-time)
>>  
>>  (eval-when-compile (require 'cl))
>
> Why not put the require in the body of nnimap-date-days-ago?

Okay, then I also need to add a defvar:
 
 (defun nnimap-date-days-ago (daysago)
   "Return date, in format \"3-Aug-1998\", for DAYSAGO days ago."
+  (require 'parse-time)
+  (defvar parse-time-months)
   (let* ((time (nnimap-time-substract (current-time) (days-to-time daysago)))
 	 (date (format-time-string
 		(format "%%d-%s-%%Y"

I have committed the changes to Gnus v5-10 branch.  If anyone want to
handle the compiler differently, feel free to change it.

Bye, Reiner.

Re: Changes in calendar/time-date.el

[Stefan Monnier]

>>> (eval-when-compile
>>>   (require 'cl)
>>>   (defvar parse-time-weekdays) ;; parse-time is required where necessary
>>>   (defvar parse-time-months)

Why not (require 'parse-time) simply?
This way if parse-time is ever changed to remove parse-time-months, the
byte-compiler will correctly catch it.

> `with-no-warnings' isn't available in Emacs 21.

with-no-warnings should be avoided as much as possible since it can hide any
warning whatsoever without justification.  (defvar foo) explains to the
byte-compiler (and the human reader) *why* the warning should be skipped.

>  (defun nnimap-date-days-ago (daysago)
>    "Return date, in format \"3-Aug-1998\", for DAYSAGO days ago."
> +  (require 'parse-time)
> +  (defvar parse-time-months)

A (defvar foo) form only makes sense at the toplevel.


        Stefan

Re: Changes in calendar/time-date.el

[Kim F. Storm]

Reiner Steib <reinersteib+gmane@imap.cc> writes:

>> Can't you move this closer to the definition of message-make-date?
>> It's only necessary to suppress compiler warnings for that function.
>
> I put it inside the defun.

Doing that will slow down the function, so if that function is used
a lot, I think it is a really bad idea to do so!

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Changes in calendar/time-date.el

[Miles Bader]

On Apr 5, 2005 4:14 PM, Kim F. Storm <storm@cua.dk> wrote:
> >> Can't you move this closer to the definition of message-make-date?
> >> It's only necessary to suppress compiler warnings for that function.
> >
> > I put it inside the defun.
> 
> Doing that will slow down the function, so if that function is used
> a lot, I think it is a really bad idea to do so!

Er, no it won't.  Declaration-only defvars produce no code, whether
inside a function or no.

-Miles
-- 
Do not taunt Happy Fun Ball.

Re: Changes in calendar/time-date.el

[Kim F. Storm]

Miles Bader <snogglethorpe@gmail.com> writes:

> On Apr 5, 2005 4:14 PM, Kim F. Storm <storm@cua.dk> wrote:
>> >> Can't you move this closer to the definition of message-make-date?
>> >> It's only necessary to suppress compiler warnings for that function.
>> >
>> > I put it inside the defun.
>> 
>> Doing that will slow down the function, so if that function is used
>> a lot, I think it is a really bad idea to do so!
>
> Er, no it won't.  Declaration-only defvars produce no code, whether
> inside a function or no.

Perhaps I misread the various quoted sections.

I was refering to the (require 'parse-date)  or whatever it was called.
Putting require into a function _does_ slow it down.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Changes in calendar/time-date.el

[Lute Kamstra]

Reiner Steib <reinersteib+gmane@imap.cc> writes:

[...]

> I have committed the changes to Gnus v5-10 branch.  If anyone want to
> handle the compiler differently, feel free to change it.

I'd prefer to handle it like in the patch below.  If the function
message-make-date or nnimap-date-days-ago is called *very* often, then
a single (require 'parse-time) at top level may be better.  But all
require does is a memq on features, which has typically a few hundred
members.  I suspect that doesn't take too much time.

Lute.


Index: lisp/gnus/message.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/gnus/message.el,v
retrieving revision 1.76
diff -u -r1.76 message.el
--- lisp/gnus/message.el	5 Apr 2005 04:10:27 -0000	1.76
+++ lisp/gnus/message.el	5 Apr 2005 13:42:31 -0000
@@ -4564,12 +4564,11 @@
 	(when (re-search-forward ",+$" nil t)
 	  (replace-match "" t t))))))
 
+(eval-when-compile (require 'parse-time))
 (defun message-make-date (&optional now)
   "Make a valid data header.
 If NOW, use that time instead."
   (require 'parse-time)
-  (defvar parse-time-weekdays)
-  (defvar parse-time-months)
   (let* ((now (or now (current-time)))
 	 (zone (nth 8 (decode-time now)))
 	 (sign "+"))
Index: lisp/gnus/nnimap.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/gnus/nnimap.el,v
retrieving revision 1.24
diff -u -r1.24 nnimap.el
--- lisp/gnus/nnimap.el	5 Apr 2005 04:10:27 -0000	1.24
+++ lisp/gnus/nnimap.el	5 Apr 2005 13:42:33 -0000
@@ -1386,10 +1386,10 @@
 	(list (- ms 1) (+ (expt 2 16) ls))
       (list ms ls))))
 
+(eval-when-compile (require 'parse-time))
 (defun nnimap-date-days-ago (daysago)
   "Return date, in format \"3-Aug-1998\", for DAYSAGO days ago."
   (require 'parse-time)
-  (defvar parse-time-months)
   (let* ((time (nnimap-time-substract (current-time) (days-to-time daysago)))
 	 (date (format-time-string
 		(format "%%d-%s-%%Y"

require inside functions. (was: Changes in calendar/time-date.el)

[Lute Kamstra]

storm@cua.dk (Kim F. Storm) writes:

[...]

> Putting require into a function _does_ slow it down.

I decided to test this.  The speed effect is really minimal, but I did
discover a more serious problem with putting a require inside a
function that is called often.

(require 'ft) only loads a file if 'ft is not in features.  However,
it unconditionally adds '(require . ft) to current-load-list.  If you
call a function with require a million times, this eats up 16 MB of
memory.

Should this be fixed somehow?

Lute.

Re: require inside functions. (was: Changes in calendar/time-date.el)

[Kim F. Storm]

Lute Kamstra <Lute.Kamstra.lists@xs4all.nl> writes:

> storm@cua.dk (Kim F. Storm) writes:
>
> [...]
>
>> Putting require into a function _does_ slow it down.
>
> I decided to test this.  The speed effect is really minimal, 

I guess it depends on how deep down in the require alist, the symbol
is located.  I.e. if the package was not loaded before you run your
function, it will be first in the list, and so it doesn't have much
impact.

> (require 'ft) only loads a file if 'ft is not in features.  However,
> it unconditionally adds '(require . ft) to current-load-list.  If you
> call a function with require a million times, this eats up 16 MB of
> memory.

The problem seems to be that current-load-list is never truncated.
But I don't quite understand what current-load-list is good for
(outside the byte compiler).

>
> Should this be fixed somehow?

I would think so...

On my system, current-load-list contains the following after reading a few mails and doing M-x grep a few times.

current-load-list
((require . parse-time) (require . compile) (require . compile) (require . parse-time) (require . parse-time) (require . parse-time) (require . parse-time) (require . compile) (require . parse-time) (require . compile) (require . parse-time) (require . parse-time) (require . compile) (require . parse-time) (require . compile) (require . parse-time) (require . parse-time) (require . parse-time) (require . parse-time) (require . gnus-sum) (require  sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . sort) (require . nnml) (require . message) (require . message) (require . message) (require . message) (require . message) (require . message) (require . message) (require . message) (require . message) (require . message) (require . message) (require . message) (require . mail-utils) (require . mail-utils) (require . message) (require . message) (require . message) (require . message) (require . message) (require . message) (require . message) (require . message) (require . message) (require . message) (require . message) (require . parse-time) (require . parse-time) (require . mail-utils) (require . parse-time) (require . parse-time) (require . gnus-sum) (defun . message-make-address) (defun . message-make-sender) (require . parse-time) (require . parse-time) (require . tool-bar) (require . gnus-sum) (require . vc-cvs) (require . parse-time) (require . sort) (require . sort) (require . tool-bar) (require . nnmail) (require . nnmail) (require . nndoc) (require . nnmail) (require . nnmail) (require . parse-time) (require . nnmail) (require . nnfolder) (defun . gnus-byte-compile) (require . bytecomp) (require . byte-optimize) (require . nnml) (require . nnmail) (require . nnmail) (require . nndraft) (require . nntp) (provide . make-network-process))


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: require inside functions.

[Miles Bader]

storm@cua.dk (Kim F. Storm) writes:
>> (require 'ft) only loads a file if 'ft is not in features.  However,
>> it unconditionally adds '(require . ft) to current-load-list.  If you
>> call a function with require a million times, this eats up 16 MB of
>> memory.
>
> The problem seems to be that current-load-list is never truncated.
> But I don't quite understand what current-load-list is good for
> (outside the byte compiler).

I don't understand it either, but ... if require doesn't actually load
the file, surely nothing at all should be added to `current-load-list'?

This seems like an out-and-out bug [and I notice my `current-load-list'
is also filled up entirely with largely redundant (require . xxx) entries.]

-Miles
-- 
Next to fried food, the South has suffered most from oratory.
  			-- Walter Hines Page

Re: require inside functions.

[Stefan Monnier]

> The problem seems to be that current-load-list is never truncated.
> But I don't quite understand what current-load-list is good for
> (outside the byte compiler).

How 'bout load-history?


        Stefan

Re: require inside functions.

[Stefan Monnier]

> I don't understand it either, but ... if require doesn't actually load
> the file, surely nothing at all should be added to `current-load-list'?

current-load-list is used to build load-history and it was recently
discovered that the (require . foo) entries should be added there, whether
the require did cause a load or not.


        Stefan

Re: require inside functions.

[Miles Bader]

On Apr 8, 2005 9:45 AM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> > I don't understand it either, but ... if require doesn't actually load
> > the file, surely nothing at all should be added to `current-load-list'?
> 
> current-load-list is used to build load-history and it was recently
> discovered that the (require . foo) entries should be added there, whether
> the require did cause a load or not.

Can you give a pointer to the discussion where it was discovered?  I
find it quite counter-intuitive (my intuition says "require should be
a nop for an already-loaded feature").

-Miles
-- 
Do not taunt Happy Fun Ball.

Re: require inside functions. (was: Changes in calendar/time-date.el)

[Richard Stallman]

    (require 'ft) only loads a file if 'ft is not in features.  However,
    it unconditionally adds '(require . ft) to current-load-list.  If you
    call a function with require a million times, this eats up 16 MB of
    memory.

This was done deliberately.  The idea is that it's useful
to record that file foo depends on file bar, even if bar
was already loaded before foo.

However, it isn't useful to record (require . bar) an additional time
in current-load-list when it's already there.  So I think the right fix
is to check with Fmember and not add it a second time.

Re: require inside functions. (was: Changes in calendar/time-date.el)

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     (require 'ft) only loads a file if 'ft is not in features.  However,
>     it unconditionally adds '(require . ft) to current-load-list.  If you
>     call a function with require a million times, this eats up 16 MB of
>     memory.
>
> This was done deliberately.  The idea is that it's useful
> to record that file foo depends on file bar, even if bar
> was already loaded before foo.

It is not _file_ foo, but _function_ foo that did (require 'bar).

Why is it useful to record that?  For what purpose?

There is no active "load" or "autoload" in progress, so the
require is done in "interactive" context.


>
> However, it isn't useful to record (require . bar) an additional time
> in current-load-list when it's already there.  So I think the right fix
> is to check with Fmember and not add it a second time.

Which means that (require 'bar) called from a function will definitely
be slowed down (depending on the length of current-load-list)!

Would it work to only add to current-load-list if loading != 0 ?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: require inside functions.

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

>     (require 'ft) only loads a file if 'ft is not in features.  However,
>     it unconditionally adds '(require . ft) to current-load-list.  If you
>     call a function with require a million times, this eats up 16 MB of
>     memory.
>
> This was done deliberately.  The idea is that it's useful
> to record that file foo depends on file bar, even if bar
> was already loaded before foo.
>
> However, it isn't useful to record (require . bar) an additional
> time in current-load-list when it's already there.  So I think the
> right fix is to check with Fmember and not add it a second time.

Probably a performance issue?  Maybe the check for it already being
present should be done using a hashtable, say current-load-hashtable?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: require inside functions. (was: Changes in

[Richard Stallman]

    It is not _file_ foo, but _function_ foo that did (require 'bar).

    Why is it useful to record that?  For what purpose?

It isn't particularly useful, but it is hard to avoid.
Frequire does the same things, whether `require' was called
from a Lisp function or from code at top level in a file.

However, my change turns off recording in the case where
no file is being loaded.

Does this give good results?

*** fns.c	18 Jan 2005 19:52:01 -0500	1.382
--- fns.c	08 Apr 2005 22:22:22 -0400	
***************
*** 66,71 ****
--- 66,72 ----
  extern int minibuffer_auto_raise;
  extern Lisp_Object minibuf_window;
  extern Lisp_Object Vlocale_coding_system;
+ extern Lisp_Object Vloads_in_progress;
  
  Lisp_Object Qstring_lessp, Qprovide, Qrequire;
  Lisp_Object Qyes_or_no_p_history;
***************
*** 3444,3452 ****
    CHECK_SYMBOL (feature);
  
    /* Record the presence of `require' in this file
!      even if the feature specified is already loaded.  */
!   LOADHIST_ATTACH (Fcons (Qrequire, feature));
! 
    tem = Fmemq (feature, Vfeatures);
  
    if (NILP (tem))
--- 3445,3459 ----
    CHECK_SYMBOL (feature);
  
    /* Record the presence of `require' in this file
!      even if the feature specified is already loaded.
!      But not more than once in any file,
!      and not when we aren't loading a file.  */
!   if (! NILP (Vloads_in_progress))
!     {
!       tem = Fcons (Qrequire, feature);
!       if (NILP (Fmember (tem, Vcurrent_load_list)))
! 	LOADHIST_ATTACH (tem);
!     }
    tem = Fmemq (feature, Vfeatures);
  
    if (NILP (tem))

Re: require inside functions.

[Lute Kamstra]

Richard Stallman <rms@gnu.org> writes:

>     It is not _file_ foo, but _function_ foo that did (require 'bar).
>
>     Why is it useful to record that?  For what purpose?
>
> It isn't particularly useful, but it is hard to avoid.
> Frequire does the same things, whether `require' was called
> from a Lisp function or from code at top level in a file.
>
> However, my change turns off recording in the case where
> no file is being loaded.
>
> Does this give good results?
>
> *** fns.c	18 Jan 2005 19:52:01 -0500	1.382
> --- fns.c	08 Apr 2005 22:22:22 -0400	
> ***************
> *** 66,71 ****
> --- 66,72 ----
>   extern int minibuffer_auto_raise;
>   extern Lisp_Object minibuf_window;
>   extern Lisp_Object Vlocale_coding_system;
> + extern Lisp_Object Vloads_in_progress;
>   
>   Lisp_Object Qstring_lessp, Qprovide, Qrequire;
>   Lisp_Object Qyes_or_no_p_history;

Vloads_in_progress currently has internal linkage in lread.c.

> ***************
> *** 3444,3452 ****
>     CHECK_SYMBOL (feature);
>   
>     /* Record the presence of `require' in this file
> !      even if the feature specified is already loaded.  */
> !   LOADHIST_ATTACH (Fcons (Qrequire, feature));
> ! 
>     tem = Fmemq (feature, Vfeatures);
>   
>     if (NILP (tem))
> --- 3445,3459 ----
>     CHECK_SYMBOL (feature);
>   
>     /* Record the presence of `require' in this file
> !      even if the feature specified is already loaded.
> !      But not more than once in any file,
> !      and not when we aren't loading a file.  */
> !   if (! NILP (Vloads_in_progress))
> !     {
> !       tem = Fcons (Qrequire, feature);
> !       if (NILP (Fmember (tem, Vcurrent_load_list)))
> ! 	LOADHIST_ATTACH (tem);
> !     }
>     tem = Fmemq (feature, Vfeatures);
>   
>     if (NILP (tem))

For the rest, the patch works fine in the sense that (1)
current-load-list does not grow anymore during a normal Emacs run and
(2) require is as fast as featurep in case the feature is already
loaded.  But I don't know the internals of the load process, so I
can't say if the patch breaks anything there.

Lute.

Re: require inside functions.

[Richard Stallman]

    Vloads_in_progress currently has internal linkage in lread.c.

Thanks.  I will fix that when I install it.

      But I don't know the internals of the load process, so I
    can't say if the patch breaks anything there.

I don't think so.

Re: require inside functions.

[Lute Kamstra]

Richard Stallman <rms@gnu.org> writes:

[...]

> Does this give good results?
>
> *** fns.c	18 Jan 2005 19:52:01 -0500	1.382
> --- fns.c	08 Apr 2005 22:22:22 -0400	
> ***************
> *** 66,71 ****
> --- 66,72 ----
>   extern int minibuffer_auto_raise;
>   extern Lisp_Object minibuf_window;
>   extern Lisp_Object Vlocale_coding_system;
> + extern Lisp_Object Vloads_in_progress;
>   
>   Lisp_Object Qstring_lessp, Qprovide, Qrequire;
>   Lisp_Object Qyes_or_no_p_history;
> ***************
> *** 3444,3452 ****
>     CHECK_SYMBOL (feature);
>   
>     /* Record the presence of `require' in this file
> !      even if the feature specified is already loaded.  */
> !   LOADHIST_ATTACH (Fcons (Qrequire, feature));
> ! 
>     tem = Fmemq (feature, Vfeatures);
>   
>     if (NILP (tem))
> --- 3445,3459 ----
>     CHECK_SYMBOL (feature);
>   
>     /* Record the presence of `require' in this file
> !      even if the feature specified is already loaded.
> !      But not more than once in any file,
> !      and not when we aren't loading a file.  */
> !   if (! NILP (Vloads_in_progress))
> !     {
> !       tem = Fcons (Qrequire, feature);
> !       if (NILP (Fmember (tem, Vcurrent_load_list)))
> ! 	LOADHIST_ATTACH (tem);
> !     }
>     tem = Fmemq (feature, Vfeatures);
>   
>     if (NILP (tem))

Did you perhaps mean to use load_in_progress instead of Vloads_in_progress?

Lute.


Index: src/fns.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/fns.c,v
retrieving revision 1.384
diff -c -r1.384 fns.c
*** src/fns.c	10 Apr 2005 19:02:24 -0000	1.384
--- src/fns.c	13 Apr 2005 08:42:48 -0000
***************
*** 66,72 ****
  extern int minibuffer_auto_raise;
  extern Lisp_Object minibuf_window;
  extern Lisp_Object Vlocale_coding_system;
! extern Lisp_Object Vloads_in_progress;
  
  Lisp_Object Qstring_lessp, Qprovide, Qrequire;
  Lisp_Object Qyes_or_no_p_history;
--- 66,72 ----
  extern int minibuffer_auto_raise;
  extern Lisp_Object minibuf_window;
  extern Lisp_Object Vlocale_coding_system;
! extern int load_in_progress;
  
  Lisp_Object Qstring_lessp, Qprovide, Qrequire;
  Lisp_Object Qyes_or_no_p_history;
***************
*** 3460,3466 ****
       even if the feature specified is already loaded.
       But not more than once in any file,
       and not when we aren't loading a file.  */
!   if (! NILP (Vloads_in_progress))
      {
        tem = Fcons (Qrequire, feature);
        if (NILP (Fmember (tem, Vcurrent_load_list)))
--- 3460,3466 ----
       even if the feature specified is already loaded.
       But not more than once in any file,
       and not when we aren't loading a file.  */
!   if (load_in_progress)
      {
        tem = Fcons (Qrequire, feature);
        if (NILP (Fmember (tem, Vcurrent_load_list)))
Index: src/lread.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/lread.c,v
retrieving revision 1.329
diff -c -r1.329 lread.c
*** src/lread.c	10 Apr 2005 00:28:37 -0000	1.329
--- src/lread.c	13 Apr 2005 08:42:51 -0000
***************
*** 1,6 ****
  /* Lisp parsing and input streams.
     Copyright (C) 1985, 1986, 1987, 1988, 1989, 1993, 1994, 1995, 1997, 1998,
!      1999, 2000, 2001, 2003, 2004  Free Software Foundation, Inc.
  
  This file is part of GNU Emacs.
  
--- 1,6 ----
  /* Lisp parsing and input streams.
     Copyright (C) 1985, 1986, 1987, 1988, 1989, 1993, 1994, 1995, 1997, 1998,
!      1999, 2000, 2001, 2003, 2004, 2005  Free Software Foundation, Inc.
  
  This file is part of GNU Emacs.
  
***************
*** 193,199 ****
  /* A list of file names for files being loaded in Fload.  Used to
     check for recursive loads.  */
  
! Lisp_Object Vloads_in_progress;
  
  /* Non-zero means load dangerous compiled Lisp files.  */
  
--- 193,199 ----
  /* A list of file names for files being loaded in Fload.  Used to
     check for recursive loads.  */
  
! static Lisp_Object Vloads_in_progress;
  
  /* Non-zero means load dangerous compiled Lisp files.  */

Re: require inside functions.

[Richard Stallman]

    Did you perhaps mean to use load_in_progress instead of Vloads_in_progress?

I meant to use Vloads_in_progress.  Would load_in_progress do the same
job better?  If so, I don't mind changing it.

Re: require inside functions.

[Lute Kamstra]

Richard Stallman <rms@gnu.org> writes:

>     Did you perhaps mean to use load_in_progress instead of Vloads_in_progress?
>
> I meant to use Vloads_in_progress.  Would load_in_progress do the same
> job better?  If so, I don't mind changing it.

I browsed src/lread.c a bit and they seem equivalent in that you can
use both of them to check if a load is in progress, which is the goal
in Frequire.  It's just that with load_in_progress the check is
simpler.  Using load_in_progress is also clearer due to the
documentation/purpose of both variables:

/* non-zero if inside `load' */
int load_in_progress;

/* A list of file names for files being loaded in Fload.  Used to
   check for recursive loads.  */
static Lisp_Object Vloads_in_progress;


Lute.