Re: master d0c77a1: Remove some assumptions about timestamp format

Re: master d0c77a1: Remove some assumptions about timestamp format

[Michael Albinus]

eggert@cs.ucla.edu (Paul Eggert) writes:

Hi Paul,

>     Remove some assumptions about timestamp format
>     
>     These changes remove some assumptions of Lisp code on timestamp
>     format.  Although we’re not going to change the default format any
>     time soon, I went looking for code that was too intimate about
>     details of timestamp format and removed assumptions where this was
>     easy to do with current Emacs primitives.
>     * test/lisp/calendar/parse-time-tests.el (parse-time-tests):
>     * test/lisp/emacs-lisp/timer-tests.el (timer-test-multiple-of-time):
>     * test/lisp/net/tramp-tests.el:
>     (tramp-test19-directory-files-and-attributes)
>     (tramp-test22-file-times, tramp-test23-visited-file-modtime):
>     Don’t assume detailed format of returned Lisp timestamps.

> diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
> index 55884f3..7901355 100644
> --- a/test/lisp/net/tramp-tests.el
> +++ b/test/lisp/net/tramp-tests.el
> @@ -2882,16 +2882,16 @@ This tests also `file-readable-p', `file-regular-p' and
>  	    ;; able to return the date correctly.  They say "don't know".
>  	    (dolist (elt attr)
>  	      (unless
> -		  (equal
> -		   (nth
> -		    5 (file-attributes (expand-file-name (car elt) tmp-name2)))
> -		   '(0 0))
> +		  (zerop
> +		   (float-time
> +		    (nth 5 (file-attributes
> +			    (expand-file-name (car elt) tmp-name2)))))
>  		(should
>  		 (equal (file-attributes (expand-file-name (car elt) tmp-name2))
>  			(cdr elt)))))
>  	    (setq attr (directory-files-and-attributes tmp-name2 'full))
>  	    (dolist (elt attr)
> -	      (unless (equal (nth 5 (file-attributes (car elt))) '(0 0))
> +	      (unless (zerop (float-time (nth 5 (file-attributes (car elt)))))
>  		(should
>  		 (equal (file-attributes (car elt)) (cdr elt)))))
>  	    (setq attr (directory-files-and-attributes tmp-name2 nil "^b"))
> @@ -3215,14 +3215,14 @@ This tests also `make-symbolic-link', `file-truename' and `add-name-to-file'."
>  	    (write-region "foo" nil tmp-name1)
>  	    (should (file-exists-p tmp-name1))
>  	    (should (consp (nth 5 (file-attributes tmp-name1))))
> -	    ;; '(0 0) means don't know, and will be replaced by
> -	    ;; `current-time'.  Therefore, we use '(0 1).  We skip the
> +	    ;; A zero timestamp means don't know, and will be replaced by
> +	    ;; `current-time'.  Therefore, use timestamp 1.  Skip the
>  	    ;; test, if the remote handler is not able to set the
>  	    ;; correct time.
>  	    (skip-unless (set-file-times tmp-name1 (seconds-to-time 1)))
>  	    ;; Dumb remote shells without perl(1) or stat(1) are not
>  	    ;; able to return the date correctly.  They say "don't know".
> -	    (unless (equal (nth 5 (file-attributes tmp-name1)) '(0 0))
> +	    (unless (zerop (float-time (nth 5 (file-attributes tmp-name1))))
>  	      (should
>  	       (equal (nth 5 (file-attributes tmp-name1)) (seconds-to-time 1)))
>  	      (write-region "bla" nil tmp-name2)
> @@ -3250,9 +3250,9 @@ This tests also `make-symbolic-link', `file-truename' and `add-name-to-file'."
>  	    (with-temp-buffer
>  	      (insert-file-contents tmp-name)
>  	      (should (verify-visited-file-modtime))
> -	      (set-visited-file-modtime '(0 1))
> +              (set-visited-file-modtime (seconds-to-time 1))
>  	      (should (verify-visited-file-modtime))
> -	      (should (equal (visited-file-modtime) '(0 1 0 0)))))
> +	      (should (= 1 (float-time (visited-file-modtime))))))
>  
>  	;; Cleanup.
>  	(ignore-errors (delete-file tmp-name))))))

In the different tramp*.el files, '(0 0) is explicitly quoted as "don't
know value". This is there for ages.

If you want to change this in the tests, it shall be changed in
tramp*.el as well. Maybe it would be even worth an own constant?
Something like

(defconst time-dont-know (seconds-to-time 0)
  "A time value indicating an unknown time.")

If you like, it could even have the value 0 instead of (seconds-to-time
0). But this would also introduce "some assumptions of Lisp code on
timestamp format" - what you try to avoid.

Best regards, Michael.

Re: master d0c77a1: Remove some assumptions about timestamp format

[Paul Eggert]

On 9/25/18 3:09 AM, Michael Albinus wrote:
> In the different tramp*.el files, '(0 0) is explicitly quoted as "don't
> know value". This is there for ages.
>
> If you want to change this in the tests, it shall be changed in
> tramp*.el as well. Maybe it would be even worth an own constant?
> Something like
>
> (defconst time-dont-know (seconds-to-time 0)
>    "A time value indicating an unknown time.")
>
> If you like, it could even have the value 0 instead of (seconds-to-time
> 0). But this would also introduce "some assumptions of Lisp code on
> timestamp format" - what you try to avoid.


A constant like that would make things clearer, yes. Either 0 or (0 0 0 
0) or (0 0) should do, so I suggest just 0 as it's simplest. While 
you're at it, you might consider changing the number from 0 to a 
negative value, say -1 (the value that POSIX uses for invalid time_t), 
since that's less likely to collide with actual file timestamps 
(timestamp 0 is far more common than timestamp -1 in real filesystems in 
my experience).

The Tramp code shouldn't care whether the constant is 0 or (0 0) or (0 0 
0 0) [or -1 or (-1 65535) or (-1 65535 0 0)], because the code should 
compare time stamps via time-less-p (or float-time, if the timestamps 
are known to be small like 0 or -1, or maybe we should add time-equal?), 
not via 'equal'. The idea is that the Lisp timestamp format has changed 
in the past and is likely to change in the future and one should not 
assume any particular format if one wants the code to be portable.

Re: master d0c77a1: Remove some assumptions about timestamp format

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

> A constant like that would make things clearer, yes. Either 0 or (0 0
> 0 0) or (0 0) should do, so I suggest just 0 as it's simplest. While
> you're at it, you might consider changing the number from 0 to a
> negative value, say -1 (the value that POSIX uses for invalid time_t),
> since that's less likely to collide with actual file timestamps
> (timestamp 0 is far more common than timestamp -1 in real filesystems
> in my experience).

There is already invalid_timespec() in systime.h. What about the
following patch?

--8<---------------cut here---------------start------------->8---
diff --git a/src/editfns.c b/src/editfns.c
index 8c7491beed..fc076a52c1 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -1592,6 +1592,10 @@ static Lisp_Object
 time_arith (Lisp_Object a, Lisp_Object b,
 	    struct lisp_time (*op) (struct lisp_time, struct lisp_time))
 {
+  if (!NILP (Ftime_equal_p (a, Vinvalid_time)) ||
+      !NILP (Ftime_equal_p (b, Vinvalid_time)))
+    return Vinvalid_time;
+
   int alen, blen;
   struct lisp_time ta = lisp_time_struct (a, &alen);
   struct lisp_time tb = lisp_time_struct (b, &blen);
@@ -1620,6 +1624,7 @@ time_arith (Lisp_Object a, Lisp_Object b,
 DEFUN ("time-add", Ftime_add, Stime_add, 2, 2, 0,
        doc: /* Return the sum of two time values A and B, as a time value.
 A nil value for either argument stands for the current time.
+If time value A or B is equal to `invalid-time', this value is returned.
 See `current-time-string' for the various forms of a time value.  */)
   (Lisp_Object a, Lisp_Object b)
 {
@@ -1630,6 +1635,7 @@ DEFUN ("time-subtract", Ftime_subtract, Stime_subtract, 2, 2, 0,
        doc: /* Return the difference between two time values A and B, as a time value.
 Use `float-time' to convert the difference into elapsed seconds.
 A nil value for either argument stands for the current time.
+If time value A or B is equal to `invalid-time', this value is returned.
 See `current-time-string' for the various forms of a time value.  */)
   (Lisp_Object a, Lisp_Object b)
 {
@@ -1652,6 +1658,19 @@ See `current-time-string' for the various forms of a time value.  */)
 	  ? Qt : Qnil);
 }
 
+DEFUN ("time-equal-p", Ftime_equal_p, Stime_equal_p, 2, 2, 0,
+       doc: /* Return non-nil if time value T1 is equal to time value T2.
+A nil value for either argument stands for the current time.
+See `current-time-string' for the various forms of a time value.  */)
+  (Lisp_Object t1, Lisp_Object t2)
+{
+  int t1len, t2len;
+  struct lisp_time a = lisp_time_struct (t1, &t1len);
+  struct lisp_time b = lisp_time_struct (t2, &t2len);
+  return
+    a.hi == b.hi && a.lo == b.lo && a.us == b.us && a.ps == b.ps ? Qt : Qnil;
+}
+
 
 DEFUN ("get-internal-run-time", Fget_internal_run_time, Sget_internal_run_time,
        0, 0, 0,
@@ -2287,6 +2306,9 @@ the TZ environment variable.  It can also be a list (as from
 without consideration for daylight saving time.  */)
   (Lisp_Object specified_time, Lisp_Object zone)
 {
+  if (Ftime_equal_p (specified_time, Vinvalid_time))
+      check_time_validity (0);
+
   time_t value = lisp_seconds_argument (specified_time);
   timezone_t tz = tzlookup (zone, false);
 
@@ -5661,6 +5683,10 @@ it to be non-nil.  */);
   binary_as_unsigned = true;
 #endif
 
+  DEFVAR_LISP ("invalid-time", Vinvalid_time,
+	       doc: /* An invalid time value, used as "Don't know" value.  */);
+  Vinvalid_time = make_lisp_time (invalid_timespec ());
+
   defsubr (&Spropertize);
   defsubr (&Schar_equal);
   defsubr (&Sgoto_char);
@@ -5734,6 +5760,7 @@ it to be non-nil.  */);
   defsubr (&Stime_add);
   defsubr (&Stime_subtract);
   defsubr (&Stime_less_p);
+  defsubr (&Stime_equal_p);
   defsubr (&Sget_internal_run_time);
   defsubr (&Sformat_time_string);
   defsubr (&Sfloat_time);
--8<---------------cut here---------------end--------------->8---

> The Tramp code shouldn't care whether the constant is 0 or (0 0) or (0
> 0 0 0) [or -1 or (-1 65535) or (-1 65535 0 0)], because the code
> should compare time stamps via time-less-p (or float-time, if the
> timestamps are known to be small like 0 or -1, or maybe we should add
> time-equal?), not via 'equal'. The idea is that the Lisp timestamp
> format has changed in the past and is likely to change in the future
> and one should not assume any particular format if one wants the code
> to be portable.

Tramp would use only `invalid-time' and `time-equal-p', agnostic to
their implementations. Plus some compatibility code for older Emacsen.

Best regards, Michael.

Re: master d0c77a1: Remove some assumptions about timestamp format

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Wed, 26 Sep 2018 11:24:51 +0200
> Cc: emacs-devel@gnu.org
> 
> There is already invalid_timespec() in systime.h. What about the
> following patch?

If this goes in, please document time-equal-p and invalid-time in NEWS
and in the ELisp manual.

TIA

Re: master d0c77a1: Remove some assumptions about timestamp format

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> If this goes in, please document time-equal-p and invalid-time in NEWS
> and in the ELisp manual.

Of course, it's on my todo. But I want the get the OK from Paul first.
(And from the maintainers, if they care :-)

> TIA

Best regards, Michael.

Re: master d0c77a1: Remove some assumptions about timestamp format

[Paul Eggert]

I'm afraid we need something better than that, as it mishandles NaNs. 
Also, there's no need for a special "invalid_time" value; you can just 
use a NaN.

Re: master d0c77a1: Remove some assumptions about timestamp format

[Paul Eggert]

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

I installed the attached, which should be a win independently of what 
Tramp needs as it adds support for time-equal, infinite and NaN time 
values. I hope it's enough for Tramp too in that it can use a NaN (or an 
infinity) to represent a timestamp that a filesystem can't possibly have.

[-- Attachment #2: 0001-time-equal-and-time-values-of-infinity-and-NaN.patch --]
[-- Type: text/x-patch, Size: 7129 bytes --]

From a588608988b5fe2c27637ad4c1783a945ea639dd Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 27 Sep 2018 18:28:27 -0700
Subject: [PATCH] time-equal, and time values of infinity and NaN

* doc/lispref/os.texi (Time Calculations):
Document time-equal, and the behavior on NaNs and infinities of
time-less-p, time-add, time-subtract.
* etc/NEWS: Mention the change.
* src/editfns.c (time_arith): Change last arg from function
to bool.  All callers changed.  Do the right thing with
infinities and NaNs.
(time_cmp): New function, which handlesx infinities and NaNs.
(Ftime_less_p): Use it.
(Ftime_equal): New function.
* test/lisp/emacs-lisp/timer-tests.el (timer-test-multiple-of-time):
Use it.
---
 doc/lispref/os.texi                 | 11 +++++-
 etc/NEWS                            |  8 ++++
 src/editfns.c                       | 58 ++++++++++++++++++++++-------
 test/lisp/emacs-lisp/timer-tests.el |  8 ++--
 4 files changed, 66 insertions(+), 19 deletions(-)

diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
index 8481fea806..67b78aea74 100644
--- a/doc/lispref/os.texi
+++ b/doc/lispref/os.texi
@@ -1736,17 +1736,26 @@ Time Calculations
 @defun time-less-p t1 t2
 This returns @code{t} if time value @var{t1} is less than time value
 @var{t2}.
+The result is @code{nil} if either argument is a NaN.
+@end defun
+
+@defun time-equal t1 t2
+This returns @code{t} if @var{t1} and @var{t2} are equal time values.
+The result is @code{nil} if either argument is a NaN.
 @end defun
 
 @defun time-subtract t1 t2
 This returns the time difference @var{t1} @minus{} @var{t2} between
-two time values, as a time value.  If you need the difference in units
+two time values, as a time value.  However, the result is a float
+if either argument is a float infinity or NaN@.
+If you need the difference in units
 of elapsed seconds, use @code{float-time} (@pxref{Time of Day,
 float-time}) to convert the result into seconds.
 @end defun
 
 @defun time-add t1 t2
 This returns the sum of two time values, as a time value.
+However, the result is a float if either argument is a float infinity or NaN@.
 One argument should represent a time difference rather than a point in time,
 either as a list or as a single number of elapsed seconds.
 Here is how to add a number of seconds to a time value:
diff --git a/etc/NEWS b/etc/NEWS
index 2a609e4027..4dd4260b29 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -951,6 +951,14 @@ like file-attributes that compute file sizes and other attributes,
 functions like process-id that compute process IDs, and functions like
 user-uid and group-gid that compute user and group IDs.
 
++++
+** 'time-add', 'time-subtract', and 'time-less-p' now accept
+infinities and NaNs too, and propagate them or return nil like
+floating-point operators do.
+
++++
+** New function 'time-equal' compares time values for equality.
+
 ** define-minor-mode automatically documents the meaning of ARG.
 
 +++
diff --git a/src/editfns.c b/src/editfns.c
index ec6e8ba98d..acd80bbf31 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -1589,13 +1589,21 @@ time_subtract (struct lisp_time ta, struct lisp_time tb)
 }
 
 static Lisp_Object
-time_arith (Lisp_Object a, Lisp_Object b,
-	    struct lisp_time (*op) (struct lisp_time, struct lisp_time))
+time_arith (Lisp_Object a, Lisp_Object b, bool subtract)
 {
+  if (FLOATP (a) && !isfinite (XFLOAT_DATA (a)))
+    {
+      double da = XFLOAT_DATA (a);
+      double db = XFLOAT_DATA (Ffloat_time (b));
+      return make_float (subtract ? da - db : da + db);
+    }
+  if (FLOATP (b) && !isfinite (XFLOAT_DATA (b)))
+    return subtract ? make_float (-XFLOAT_DATA (b)) : b;
+
   int alen, blen;
   struct lisp_time ta = lisp_time_struct (a, &alen);
   struct lisp_time tb = lisp_time_struct (b, &blen);
-  struct lisp_time t = op (ta, tb);
+  struct lisp_time t = (subtract ? time_subtract : time_add) (ta, tb);
   if (FIXNUM_OVERFLOW_P (t.hi))
     time_overflow ();
   Lisp_Object val = Qnil;
@@ -1623,7 +1631,7 @@ A nil value for either argument stands for the current time.
 See `current-time-string' for the various forms of a time value.  */)
   (Lisp_Object a, Lisp_Object b)
 {
-  return time_arith (a, b, time_add);
+  return time_arith (a, b, false);
 }
 
 DEFUN ("time-subtract", Ftime_subtract, Stime_subtract, 2, 2, 0,
@@ -1633,7 +1641,30 @@ A nil value for either argument stands for the current time.
 See `current-time-string' for the various forms of a time value.  */)
   (Lisp_Object a, Lisp_Object b)
 {
-  return time_arith (a, b, time_subtract);
+  return time_arith (a, b, true);
+}
+
+/* Return negative, 0, positive if a < b, a == b, a > b respectively.
+   Return positive if either a or b is a NaN; this is good enough
+   for the current callers.  */
+static int
+time_cmp (Lisp_Object a, Lisp_Object b)
+{
+  if ((FLOATP (a) && !isfinite (XFLOAT_DATA (a)))
+      || (FLOATP (b) && !isfinite (XFLOAT_DATA (b))))
+    {
+      double da = FLOATP (a) ? XFLOAT_DATA (a) : 0;
+      double db = FLOATP (b) ? XFLOAT_DATA (b) : 0;
+      return da < db ? -1 : da != db;
+    }
+
+  int alen, blen;
+  struct lisp_time ta = lisp_time_struct (a, &alen);
+  struct lisp_time tb = lisp_time_struct (b, &blen);
+  return (ta.hi != tb.hi ? (ta.hi < tb.hi ? -1 : 1)
+	  : ta.lo != tb.lo ? (ta.lo < tb.lo ? -1 : 1)
+	  : ta.us != tb.us ? (ta.us < tb.us ? -1 : 1)
+	  : ta.ps < tb.ps ? -1 : ta.ps != tb.ps);
 }
 
 DEFUN ("time-less-p", Ftime_less_p, Stime_less_p, 2, 2, 0,
@@ -1642,14 +1673,14 @@ A nil value for either argument stands for the current time.
 See `current-time-string' for the various forms of a time value.  */)
   (Lisp_Object t1, Lisp_Object t2)
 {
-  int t1len, t2len;
-  struct lisp_time a = lisp_time_struct (t1, &t1len);
-  struct lisp_time b = lisp_time_struct (t2, &t2len);
-  return ((a.hi != b.hi ? a.hi < b.hi
-	   : a.lo != b.lo ? a.lo < b.lo
-	   : a.us != b.us ? a.us < b.us
-	   : a.ps < b.ps)
-	  ? Qt : Qnil);
+  return time_cmp (t1, t2) < 0 ? Qt : Qnil;
+}
+
+DEFUN ("time-equal", Ftime_equal, Stime_equal, 2, 2, 0,
+       doc: /* Return non-nil if T1 and T2 are equal time values.  */)
+  (Lisp_Object t1, Lisp_Object t2)
+{
+  return time_cmp (t1, t2) == 0 ? Qt : Qnil;
 }
 
 
@@ -5734,6 +5765,7 @@ it to be non-nil.  */);
   defsubr (&Scurrent_time);
   defsubr (&Stime_add);
   defsubr (&Stime_subtract);
+  defsubr (&Stime_equal);
   defsubr (&Stime_less_p);
   defsubr (&Sget_internal_run_time);
   defsubr (&Sformat_time_string);
diff --git a/test/lisp/emacs-lisp/timer-tests.el b/test/lisp/emacs-lisp/timer-tests.el
index 1d3ba757f6..0e40cdf442 100644
--- a/test/lisp/emacs-lisp/timer-tests.el
+++ b/test/lisp/emacs-lisp/timer-tests.el
@@ -40,10 +40,8 @@
       (should (debug-timer-check)) t))
 
 (ert-deftest timer-test-multiple-of-time ()
-  (should (zerop
-	   (float-time
-	    (time-subtract
-	     (timer-next-integral-multiple-of-time '(0 0 0 1) (1+ (ash 1 53)))
-	     (list (ash 1 (- 53 16)) 1))))))
+  (should (time-equal
+	   (timer-next-integral-multiple-of-time '(0 0 0 1) (1+ (ash 1 53)))
+	   (list (ash 1 (- 53 16)) 1))))
 
 ;;; timer-tests.el ends here
-- 
2.17.1

Re: master d0c77a1: Remove some assumptions about timestamp format

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 27 Sep 2018 13:46:51 -0700
> Cc: emacs-devel@gnu.org
> 
> I'm afraid we need something better than that, as it mishandles NaNs. 
> Also, there's no need for a special "invalid_time" value; you can just 
> use a NaN.

It's OK to support NaNs in time values, but I'm uneasy with the
recommendation to use NaN as an invalid time.  IMO, NaNs should only
come out of calculations, never from initial literal values of a
program (except when the program wants to test NaN handling).

What is the problem with having a special invalid time value?

Re: master d0c77a1: Remove some assumptions about timestamp format

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> I'm afraid we need something better than that, as it mishandles NaNs. 
>> Also, there's no need for a special "invalid_time" value; you can just 
>> use a NaN.
>
> It's OK to support NaNs in time values, but I'm uneasy with the
> recommendation to use NaN as an invalid time.  IMO, NaNs should only
> come out of calculations, never from initial literal values of a
> program (except when the program wants to test NaN handling).

Indeed. A NaN is not a time value all time functions do understand. For
example,

(current-time-string 0.0e+NaN)
=> (error "Specified time is not representable")

I believe, a special string shall be returned here, like
"Unspecified time" or so.

> What is the problem with having a special invalid time value?

Tramp will use such a constant anyway, it has taken '(0 0) until now. It
will be returned by Tramp implementations of functions like
`file-attributes' or `visited-file-modtime', and must be understood by
the calling functions. `set-file-times', for example, does not
understand this, and so do the functions in ls-lisp.el, and likely in
Lisp packages in the wild.

(write-region "foo" nil "/tmp/foo")
(set-file-times "/tmp/foo" 0.0e+NaN)
=> (error "Specified time is not representable")

`set-file-times' shall simply return nil in this case, as said in the
docstring.

Therefore it would be better to a have an agreed constant, which is
understood by both Tramp and its callers.

What's wrong with the result of invalid_timespec() of systime.h, as
proposed earlier?

Best regards, Michael.

Re: master d0c77a1: Remove some assumptions about timestamp format

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

> I installed the attached, which should be a win independently of what
> Tramp needs as it adds support for time-equal, infinite and NaN time
> values. I hope it's enough for Tramp too in that it can use a NaN (or
> an infinity) to represent a timestamp that a filesystem can't possibly
> have.

Thanks. Comments:

> +@defun time-equal t1 t2

Shouldn't this be `time-equal-p', as we have already `time-less-p'?

> +This returns @code{t} if @var{t1} and @var{t2} are equal time values.
> +The result is @code{nil} if either argument is a NaN.

No. Tramp (and other functions) must check, whether a given time value
is the "don't know" value. Therefore,

(time-equal time-value 0.0e+NaN)

must return t, if time-value is a NaN. And what if either value is a
float infinity?

Best regards, Michael.

Re: master d0c77a1: Remove some assumptions about timestamp format

[Paul Eggert]

Eli Zaretskii wrote:
> What is the problem with having a special invalid time value?

It depends on how special it's supposed to be, and the properties that Michael 
needs aren't clear to me yet. Evidently NaNs won't do, but it's not clear to me 
yet what will do. I'll follow up shortly.

Re: master d0c77a1: Remove some assumptions about timestamp format

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

>> What is the problem with having a special invalid time value?
>
> It depends on how special it's supposed to be, and the properties that
> Michael needs aren't clear to me yet. Evidently NaNs won't do, but
> it's not clear to me yet what will do. I'll follow up shortly.

I have no special needs for the value of that constant. It should just
be a valid time value in the scope of Emacs 27, and all timer functions
should accept it and return plausible values. A NaN would be OK, if
accepted everywhere.

For older Emacsen, I will use my own compat version of that constant
anyway. Either '(0 0) as it is the case for the last 15 years, or '(0 -1).

Best regards, Michael.

Re: master d0c77a1: Remove some assumptions about timestamp format

[Paul Eggert]

Michael Albinus wrote:

> Indeed. A NaN is not a time value all time functions do understand. For
> example,
> 
> (current-time-string 0.0e+NaN)
> => (error "Specified time is not representable")
> 
> I believe, a special string shall be returned here, like
> "Unspecified time" or so.

I'm afraid that sort of thing won't do in general. For example, what would 
format-time-string do? Or decode-time?

> Tramp will use such a constant anyway, it has taken '(0 0) until now. It
> will be returned by Tramp implementations of functions like
> `file-attributes' or `visited-file-modtime', and must be understood by
> the calling functions. `set-file-times', for example, does not
> understand this, and so do the functions in ls-lisp.el, and likely in
> Lisp packages in the wild.

If I understand you aright, you want a time value that will be treated as valid 
by standard time functions, and yet won't be a time value that any actual 
filesystem would return. In that case I suggest using the timestamp (0 0 0 1000) 
at the Lisp level. Although this is almost exactly the same time value that your 
Vinvalid_time code was using, it doesn't require special support at the C level.

Alternatively you could use (-1 65535 999999 999999), which is exactly the same 
value that your C code was effectively using. However, if it's run through 
struct timespec by the C code it will be floored to (-1 65535 999999 999000). 
Also, it can have problems on platforms where time_t is unsigned, which POSIX 
allows, and there are a few oddball systems that do that (QNX and VMS come to 
mind); although I don't know whether Emacs runs on these systems it'd be better 
not to exclude them arbitrarily. Another option would be (0 0 0 1), but I worry 
that this also would not survive an internal round trip through struct timespec.

All struct timespec-compatible timestamps are possible timestamps in the 
filesystem I use regularly (ext4). However, (0 0 0 1000) is so unlikely I don't 
think we need to worry about this, which is in contrast to the current Tramp 
usage of (0 0) which corresponds to timestamps that I have seen on real 
filesystems for various reasons.

Re: master d0c77a1: Remove some assumptions about timestamp format

[Paul Eggert]

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

Michael Albinus wrote:
> Shouldn't this be `time-equal-p', as we have already `time-less-p'?

Thanks, as a Scheme refugee I never can remember which predicates should end in 
"-p" vs "p" vs nothing. I installed the attached.

> Tramp (and other functions) must check, whether a given time value
> is the "don't know" value. Therefore,
> 
> (time-equal time-value 0.0e+NaN)
> 
> must return t, if time-value is a NaN. And what if either value is a
> float infinity?

It appears that NaN isn't what Tramp wants. As a time value, NaN should work 
like floating-point NaN does; it should never compare numerically equal to 
anything, not even to itself (anything else would lead to even more confusion 
than what we already have :-). I hope that my other suggestion for Tramp 
suffices, so that we don't need to worry about what happens with NaNs in 
comparisons.

[-- Attachment #2: 0001-Rename-time-equal-to-time-equal-p.patch --]
[-- Type: text/x-patch, Size: 2777 bytes --]

From 857c2c271080ef62e57128f531cee6e974ca28fb Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 28 Sep 2018 10:32:41 -0700
Subject: [PATCH] Rename time-equal to time-equal-p

This is for consistency with time-less-p.
* doc/lispref/os.texi (Time Calculations), etc/NEWS:
* src/editfns.c (Ftime_equal_p, syms_of_editfns):
* test/lisp/emacs-lisp/timer-tests.el (timer-test-multiple-of-time):
Rename.
---
 doc/lispref/os.texi                 | 2 +-
 etc/NEWS                            | 2 +-
 src/editfns.c                       | 4 ++--
 test/lisp/emacs-lisp/timer-tests.el | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
index 400e6bb45c..8ce5a5ed6d 100644
--- a/doc/lispref/os.texi
+++ b/doc/lispref/os.texi
@@ -1739,7 +1739,7 @@ Time Calculations
 The result is @code{nil} if either argument is a NaN.
 @end defun
 
-@defun time-equal t1 t2
+@defun time-equal-p t1 t2
 This returns @code{t} if @var{t1} and @var{t2} are equal time values.
 The result is @code{nil} if either argument is a NaN.
 @end defun
diff --git a/etc/NEWS b/etc/NEWS
index 4dd4260b29..e6508eb60b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -957,7 +957,7 @@ infinities and NaNs too, and propagate them or return nil like
 floating-point operators do.
 
 +++
-** New function 'time-equal' compares time values for equality.
+** New function 'time-equal-p' compares time values for equality.
 
 ** define-minor-mode automatically documents the meaning of ARG.
 
diff --git a/src/editfns.c b/src/editfns.c
index acd80bbf31..daea746387 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -1676,7 +1676,7 @@ See `current-time-string' for the various forms of a time value.  */)
   return time_cmp (t1, t2) < 0 ? Qt : Qnil;
 }
 
-DEFUN ("time-equal", Ftime_equal, Stime_equal, 2, 2, 0,
+DEFUN ("time-equal-p", Ftime_equal_p, Stime_equal_p, 2, 2, 0,
        doc: /* Return non-nil if T1 and T2 are equal time values.  */)
   (Lisp_Object t1, Lisp_Object t2)
 {
@@ -5765,7 +5765,7 @@ it to be non-nil.  */);
   defsubr (&Scurrent_time);
   defsubr (&Stime_add);
   defsubr (&Stime_subtract);
-  defsubr (&Stime_equal);
+  defsubr (&Stime_equal_p);
   defsubr (&Stime_less_p);
   defsubr (&Sget_internal_run_time);
   defsubr (&Sformat_time_string);
diff --git a/test/lisp/emacs-lisp/timer-tests.el b/test/lisp/emacs-lisp/timer-tests.el
index 0e40cdf442..c5971ee768 100644
--- a/test/lisp/emacs-lisp/timer-tests.el
+++ b/test/lisp/emacs-lisp/timer-tests.el
@@ -40,7 +40,7 @@
       (should (debug-timer-check)) t))
 
 (ert-deftest timer-test-multiple-of-time ()
-  (should (time-equal
+  (should (time-equal-p
 	   (timer-next-integral-multiple-of-time '(0 0 0 1) (1+ (ash 1 53)))
 	   (list (ash 1 (- 53 16)) 1))))
 
-- 
2.17.1

Naming predicates (was: master d0c77a1: Remove some assumptions about timestamp format)

[Stefan Monnier]

>> Shouldn't this be `time-equal-p', as we have already `time-less-p'?
> Thanks, as a Scheme refugee I never can remember which predicates should end
> in "-p" vs "p" vs nothing. I installed the attached.

The rule is not that bad: use a "-p or p" suffix whenever a Schemer
would use a "?" suffix.  As for when to use "-p" vs just "p" I'd say you
can simply always use "-p" when it's something you define.


        Stefan "who likes Scheme's ?, ->, and ! naming conventions"

RE: Naming predicates (was: master d0c77a1: Remove some assumptions about timestamp format)

[Drew Adams]

> >> Shouldn't this be `time-equal-p', as we have already `time-less-p'?
> > Thanks, as a Scheme refugee I never can remember which predicates should end
> > in "-p" vs "p" vs nothing. I installed the attached.
> 
> The rule is not that bad: use a "-p or p" suffix whenever a Schemer
> would use a "?" suffix.  As for when to use "-p" vs just "p" I'd say you
> can simply always use "-p" when it's something you define.

I'd say that as a user you can use whatever you want - i.e., "when it's something you define".

But there is an Emacs convention, and it's not difficult to find or to follow, for those so inclined:

In the Elisp manual, `i convention' shows you this index-entry candidate (*), among others: 

  `coding conventions in Emacs Lisp'

And choosing that takes you directly to, well, the coding conventions for Emacs Lisp (!), where you find this:

  If the purpose of a function is to tell you whether a certain
  condition is true or false, give the function a name that ends in
  'p' (which stands for "predicate").  If the name is one word, add
  just 'p'; if the name is multiple words, add '-p'.  Examples are
  'framep' and 'frame-live-p'.

_______
* Well, it shows you that candidate if you've configured Emacs option `completion-styles' to include substring matching. Or if you use something like Icicles. If not then, because that index entry doesn't start with "convention", it won't show up. In that case, you need to know to try `coding TAB' instead of `convention TAB'.

Re: Naming predicates (was: master d0c77a1: Remove some assumptions about timestamp format)

[Paul Eggert]

On 9/28/18 11:28 AM, Drew Adams wrote:
>    If the purpose of a function is to tell you whether a certain
>    condition is true or false, give the function a name that ends in
>    'p' (which stands for "predicate").  If the name is one word, add
>    just 'p'; if the name is multiple words, add '-p'.  Examples are
>    'framep' and 'frame-live-p'.

Thanks for the pointer; I'd forgotten that. But there are several 
counterexamples:

indivisible-p interactive-p ring-p registerv-p

and lots more counterexamples the other way:

bool-vector-subsetp coding-system-lessp default-boundp 
file-attributes-lessp hack-one-local-variable-eval-safep ...

Plus lots more symbols taken from Common Lisp, like cl-equalp.

The Common Lisp influence seems to be causing a lot of Elisp code to 
violate the guideline, which may help to explain why I can never 
remember it.

Re: Naming predicates

[Stefan Monnier]

>>    If the purpose of a function is to tell you whether a certain
>>    condition is true or false, give the function a name that ends in
>>    'p' (which stands for "predicate").  If the name is one word, add
>>    just 'p'; if the name is multiple words, add '-p'.  Examples are
>>    'framep' and 'frame-live-p'.

*Users* should never define a single-word function, since they should
use a namespace prefix.  Hence "I'd say you can simply always use "-p"
when it's something you define".

> Thanks for the pointer; I'd forgotten that. But there are several
> counterexamples:

You don't have to follow them when you define your own function.

> indivisible-p interactive-p ring-p registerv-p
> bool-vector-subsetp coding-system-lessp default-boundp file-attributes-lessp
> hack-one-local-variable-eval-safep ...

Yes, there are conflicting desires: when you define something that's
very much like "foop" but in the context of "strigles", the multi-word
rule suggest you name it "strigle-foo-p", but there's also an argument
to be made for "strigles-foop".

As the author of that function you get to follow whichever of the two
you like.  Personally, I think always using "-p" is a simpler rule and
is never wrong (that's the rule followed by `cl-defstruct`,
incidentally).  Even using "-p" on single-word thingies is
perfectly acceptable.

> The Common Lisp influence seems to be causing a lot of Elisp code to violate
> the guideline, which may help to explain why I can never remember it.

While the inconsistency is annoying when trying to reference an existing
function, the situation is much simpler when you're defining a new
function.


        Stefan

RE: Naming predicates

[Drew Adams]

> >>    If the purpose of a function is to tell you whether a certain
> >>    condition is true or false, give the function a name that ends in
> >>    'p' (which stands for "predicate").  If the name is one word, add
> >>    just 'p'; if the name is multiple words, add '-p'.  Examples are
> >>    'framep' and 'frame-live-p'.
> 
> *Users* should never define a single-word function, since they should
> use a namespace prefix.  Hence "I'd say you can simply always use "-p"
> when it's something you define".

Users are free to do _anything_ they like. As I said, "as a user you
can use whatever you want."

And users who write Lisp, creating a predicate, are not necessarily
writing a package or library, and they might well not want to use
a "namespace prefix."

And I believe (but I don't have a reference) that RMS pointed
out that the single-word recommendation does not include
a "namespace prefix". (Logically, it should not. IMO.)

Another consideration might be a multiple-word name that is
based on an existing name. It could be helpful to keep the
same name as a part, instead of adding a hyphen. 

> You don't have to follow them when you define your own function.

You never have to follow them, if you're not providing code
for the Emacs distribution. Does even code provided for
Emacs itself _have_ to follow them? If so, clearly that "have
to" has been bypassed or missed sometimes.

> > indivisible-p interactive-p ring-p registerv-p
> > bool-vector-subsetp coding-system-lessp default-boundp file-attributes-lessp
> > hack-one-local-variable-eval-safep ...
> 
> Yes, there are conflicting desires: when you define something that's
> very much like "foop" but in the context of "strigles", the multi-word
> rule suggest you name it "strigle-foo-p", but there's also an argument
> to be made for "strigles-foop".

Yes, that's just what I meant above.

> As the author of that function you get to follow whichever of the two
> you like.  Personally, I think always using "-p" is a simpler rule and
> is never wrong (that's the rule followed by `cl-defstruct`,
> incidentally).  Even using "-p" on single-word thingies is
> perfectly acceptable.

Acceptable does not mean conventional.

RE: Naming predicates (was: master d0c77a1: Remove some assumptions about timestamp format)

[Drew Adams]

> >    If the purpose of a function is to tell you whether a certain
> >    condition is true or false, give the function a name that ends in
> >    'p' (which stands for "predicate").  If the name is one word, add
> >    just 'p'; if the name is multiple words, add '-p'.  Examples are
> >    'framep' and 'frame-live-p'.
> 
> Thanks for the pointer; I'd forgotten that. But there are several
> counterexamples:

Yes, there are counterexamples of most of the conventions.

There also lots of examples of the conventions being followed.
And of course we are free to remove/abandon a convention,
if it's considered not worthwhile.

But as long as we have our defined conventions, the point is
to encourage their use, and so to discourage more and more
such counterexamples.

There could presumably (e.g., in the future) be some
intentional counterexamples, i.e., based on some reasons.
I don't know of any such so far, but the possibility is there.
We don't worship hobgoblin consistency for its own sake,
above all other considerations. ;-)

Other than that possibility, I expect that the existing
counterexamples are historical or accidental, where
"accidental" means unintended by the author and not
caught and corrected by reviewers. Stuff happens.

> The Common Lisp influence seems to be causing a lot of Elisp code to
> violate the guideline, which may help to explain why I can never
> remember it.

Indeed, Emacs Lisp and Emacs have their own conventions.

In some cases Emacs Lisp has a convention where Common
Lisp has none (nothing comes immediately to mind, but I'm
pretty sure there are some examples).

And vice versa. A good example is the Common Lisp
convention to name "special" variables (defvars) `*...*'.
Elisp has no convention for such naming.

Re: master d0c77a1: Remove some assumptions about timestamp format

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

> All struct timespec-compatible timestamps are possible timestamps in
> the filesystem I use regularly (ext4). However, (0 0 0 1000) is so
> unlikely I don't think we need to worry about this, which is in
> contrast to the current Tramp usage of (0 0) which corresponds to
> timestamps that I have seen on real filesystems for various reasons.

I've done so, thanks.

Best regards, Michael.