PATCH: SECONDS can be floating point

PATCH: SECONDS can be floating point

[Peter Stephenson]

This allows typeset to switch the SECONDS special parameter between
integer and floating point, with evident increase of precision (which is
not the same as accuracy!)  This method preserves backward compatability
and doesn't require any additions to the interface.  The downside is
that as usual the logic in `typeset' is rather hairy --- I've already
had a battle with things like
  fn() { local -F SECONDS; }

It's also possible that you may be using functions which assume SECONDS
is an integer, hence it might be safest not to change the type globally.
Looks like the functions we already have with SECONDS in are robust
about this (though I haven't tested explicitly).

Index: Doc/Zsh/params.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/params.yo,v
retrieving revision 1.16
diff -u -r1.16 params.yo
--- Doc/Zsh/params.yo	5 Aug 2002 12:33:28 -0000	1.16
+++ Doc/Zsh/params.yo	28 Oct 2002 18:51:35 -0000
@@ -589,6 +589,12 @@
 is assigned a value, then the value returned upon reference
 will be the value that was assigned plus the number of seconds
 since the assignment.
+
+Unlike other special parameters, the type of the tt(SECONDS) parameter can
+be changed using the tt(typeset) command.  Only integer and one of the
+floating point types are allowed.  For example, `tt(typeset -F SECONDS)'
+causes the value to be reported as a floating point number.  The precision
+is six decimal places, although not all places may be useful.
 )
 vindex(SHLVL)
 item(tt(SHLVL) <S>)(
Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.86
diff -u -r1.86 builtin.c
--- Src/builtin.c	19 Sep 2002 17:57:57 -0000	1.86
+++ Src/builtin.c	28 Oct 2002 18:51:35 -0000
@@ -1653,6 +1653,13 @@
     return &asg;
 }
 
+/* for new special parameters */
+enum {
+    NS_NONE,
+    NS_NORMAL,
+    NS_SECONDS
+};
+
 /* function to set a single parameter */
 
 /**/
@@ -1661,7 +1668,7 @@
 	       int on, int off, int roff, char *value, Param altpm,
 	       Options ops, int auxlen)
 {
-    int usepm, tc, keeplocal = 0, newspecial = 0;
+    int usepm, tc, keeplocal = 0, newspecial = NS_NONE, readonly;
     char *subscript;
 
     /*
@@ -1694,14 +1701,15 @@
 	 * local.  It can be applied either to the special or in the
 	 * typeset/local statement for the local variable.
 	 */
-	newspecial = (pm->flags & PM_SPECIAL)
-	    && !(on & PM_HIDE) && !(pm->flags & PM_HIDE & ~off);
+	if ((pm->flags & PM_SPECIAL)
+	    && !(on & PM_HIDE) && !(pm->flags & PM_HIDE & ~off))
+	    newspecial = NS_NORMAL;
 	usepm = 0;
     }
 
     /* attempting a type conversion, or making a tied colonarray? */
     tc = 0;
-    if (usepm || newspecial) {
+    if (usepm || newspecial != NS_NONE) {
 	int chflags = ((off & pm->flags) | (on & ~pm->flags)) &
 	    (PM_INTEGER|PM_EFLOAT|PM_FFLOAT|PM_HASHED|
 	     PM_ARRAY|PM_TIED|PM_AUTOLOAD);
@@ -1716,11 +1724,31 @@
      * with a special or a parameter which isn't loaded yet (which
      * may be special when it is loaded; we can't tell yet).
      */
-    if (tc || ((usepm || newspecial) && (off & pm->flags & PM_READONLY))) {
+    if ((readonly =
+	 ((usepm || newspecial != NS_NONE) && 
+	  (off & pm->flags & PM_READONLY))) ||
+	tc) {
 	if (pm->flags & PM_SPECIAL) {
-	    zerrnam(cname, "%s: can't change type of a special parameter",
-		    pname, 0);
-	    return NULL;
+	    int err = 1;
+	    if (!readonly && !strcmp(pname, "SECONDS"))
+	    {
+		if (newspecial != NS_NONE)
+		{
+		    newspecial = NS_SECONDS;
+		    err = 0;	/* and continue */
+		    tc = 0;	/* but don't do a normal conversion */
+		} else if (!setsecondstype(pm, on, off)) {
+		    if (value && !setsparam(pname, ztrdup(value)))
+			return NULL;
+		    return pm;
+		}
+	    }
+	    if (err)
+	    {
+		zerrnam(cname, "%s: can't change type of a special parameter",
+			pname, 0);
+		return NULL;
+	    }
 	} else if (pm->flags & PM_AUTOLOAD) {
 	    zerrnam(cname, "%s: can't change type of autoloaded parameter",
 		    pname, 0);
@@ -1820,7 +1848,7 @@
 	unsetparam_pm(pm, 0, 1);
     }
 
-    if (newspecial) {
+    if (newspecial != NS_NONE) {
 	Param tpm, pm2;
 	if ((pm->flags & PM_RESTRICTED) && isset(RESTRICTED)) {
 	    zerrnam(cname, "%s: restricted", pname, 0);
@@ -1866,6 +1894,8 @@
 	 * because we've checked for unpleasant surprises above.
 	 */
 	pm->flags = (PM_TYPE(pm->flags) | on | PM_SPECIAL) & ~off;
+	if (newspecial == NS_SECONDS)
+	    setsecondstype(pm, on, off);
 	/*
 	 * Final tweak: if we've turned on one of the flags with
 	 * numbers, we should use the appropriate integer.
Index: Src/params.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/params.c,v
retrieving revision 1.65
diff -u -r1.65 params.c
--- Src/params.c	5 Aug 2002 12:36:00 -0000	1.65
+++ Src/params.c	28 Oct 2002 18:51:35 -0000
@@ -141,7 +141,7 @@
 IPDEF1("HISTSIZE", histsizegetfn, histsizesetfn, PM_RESTRICTED),
 IPDEF1("RANDOM", randomgetfn, randomsetfn, 0),
 IPDEF1("SAVEHIST", savehistsizegetfn, savehistsizesetfn, PM_RESTRICTED),
-IPDEF1("SECONDS", secondsgetfn, secondssetfn, 0),
+IPDEF1("SECONDS", intsecondsgetfn, intsecondssetfn, 0),
 IPDEF1("UID", uidgetfn, uidsetfn, PM_DONTIMPORT | PM_RESTRICTED),
 IPDEF1("EUID", euidgetfn, euidsetfn, PM_DONTIMPORT | PM_RESTRICTED),
 IPDEF1("TTYIDLE", ttyidlegetfn, nullintsetfn, PM_READONLY),
@@ -2657,7 +2657,7 @@
 
 /**/
 zlong
-secondsgetfn(Param pm)
+intsecondsgetfn(Param pm)
 {
     return time(NULL) - shtimer.tv_sec;
 }
@@ -2666,12 +2666,60 @@
 
 /**/
 void
-secondssetfn(Param pm, zlong x)
+intsecondssetfn(Param pm, zlong x)
 {
     shtimer.tv_sec = time(NULL) - x;
     shtimer.tv_usec = 0;
 }
 
+/**/
+double
+floatsecondsgetfn(Param pm)
+{
+    struct timeval now;
+    struct timezone dummy_tz;
+
+    gettimeofday(&now, &dummy_tz);
+
+    return (double)(now.tv_sec - shtimer.tv_sec) +
+	(double)(now.tv_usec - shtimer.tv_usec) / 1000000.0;
+}
+
+/**/
+void
+floatsecondssetfn(Param pm, double x)
+{
+    struct timeval now;
+    struct timezone dummy_tz;
+
+    gettimeofday(&now, &dummy_tz);
+    shtimer.tv_sec = now.tv_sec - (int)x;
+    shtimer.tv_usec = now.tv_usec - (int)((x - (double)(int)x) * 1000000.0);
+}
+
+/**/
+int
+setsecondstype(Param pm, int on, int off)
+{
+    int newflags = (pm->flags | on) & ~off;
+    int tp = PM_TYPE(newflags);
+    /* Only one of the numeric types is allowed. */
+    if (tp == PM_EFLOAT || tp == PM_FFLOAT)
+    {
+	pm->gets.ffn = floatsecondsgetfn;
+	pm->sets.ffn = floatsecondssetfn;
+    }
+    else if (tp == PM_INTEGER)
+    {
+	pm->gets.ifn = intsecondsgetfn;
+	pm->sets.ifn = intsecondssetfn;
+    }
+    else
+	return 1;
+    pm->flags = newflags;
+    return 0;
+}
+
 /* Function to get value for special parameter `USERNAME' */
 
 /**/
@@ -3422,6 +3470,8 @@
 	     */
 	    Param tpm = pm->old;
 
+	    if (!strcmp(pm->nam, "SECONDS"))
+		setsecondstype(pm, PM_TYPE(tpm->flags), PM_TYPE(pm->flags));
 	    DPUTS(!tpm || PM_TYPE(pm->flags) != PM_TYPE(tpm->flags) ||
 		  !(tpm->flags & PM_SPECIAL),
 		  "BUG: in restoring scope of special parameter");


-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: SECONDS can be floating point

[Peter Stephenson]

Peter Stephenson wrote:
> This allows typeset to switch the SECONDS special parameter between
> integer and floating point.

Committed, but the following extra patch is necessary for checking that
typesets that create a local copy have a suitable type.

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.88
diff -u -r1.88 builtin.c
--- Src/builtin.c	29 Oct 2002 10:31:16 -0000	1.88
+++ Src/builtin.c	29 Oct 2002 10:39:27 -0000
@@ -1732,11 +1732,30 @@
 	    int err = 1;
 	    if (!readonly && !strcmp(pname, "SECONDS"))
 	    {
+		/*
+		 * We allow SECONDS to change type between integer
+		 * and floating point.  If we are creating a new
+		 * local copy we check the type here and allow
+		 * a new special to be created with that type.
+		 * We then need to make sure the correct type
+		 * for the special is restored at the end of the scope.
+		 * If we are changing the type of an existing
+		 * parameter, we do the whole thing here.
+		 */
 		if (newspecial != NS_NONE)
 		{
-		    newspecial = NS_SECONDS;
-		    err = 0;	/* and continue */
-		    tc = 0;	/* but don't do a normal conversion */
+		    /*
+		     * The first test allows `typeset' to copy the
+		     * existing type.  This is the usual behaviour
+		     * for making special parameters local.
+		     */
+		    if (PM_TYPE(on) == 0 || PM_TYPE(on) == PM_INTEGER ||
+			PM_TYPE(on) == PM_FFLOAT || PM_TYPE(on) == PM_EFLOAT)
+		    {
+			newspecial = NS_SECONDS;
+			err = 0;	/* and continue */
+			tc = 0;	/* but don't do a normal conversion */
+		    }
 		} else if (!setsecondstype(pm, on, off)) {
 		    if (value && !setsparam(pname, ztrdup(value)))
 			return NULL;


-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: SECONDS can be floating point

[Peter Stephenson]

While I'm at it, this makes a local SECONDS work a bit better... it's
not logically tied to the other patch but it's now more useful to have
the problem fixed, since you might want to use `typeset -F seconds' in a
function to get more precision locally.

The problem was that if you made SECONDS local, then on return to the
parent function, the time spent in the lower function didn't appear in
the value of SECONDS, hence if you were using it for overall timing ---
which is a natural thing to do --- you could miscount completely,
depending arbitrarily on the behaviour of nested functions.  This fixes
the problem by restoring SECONDS to include the elapsed time spent in
the function.  Of course, the accuracy won't be perfect, but it wouldn't
be anyway.

I think there may still be a problem if you do `typeset SECONDS=value'
inside a function that value may remain added in on return but I haven't
looked at this in detail.

Index: Src/params.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/params.c,v
retrieving revision 1.67
diff -u -r1.67 params.c
--- Src/params.c	29 Oct 2002 10:31:16 -0000	1.67
+++ Src/params.c	29 Oct 2002 11:10:16 -0000
@@ -3484,7 +3484,24 @@
 	    Param tpm = pm->old;
 
 	    if (!strcmp(pm->nam, "SECONDS"))
+	    {
 		setsecondstype(pm, PM_TYPE(tpm->flags), PM_TYPE(pm->flags));
+		/*
+		 * We restore SECONDS by adding back in the elapsed
+		 * time (from the point we reset shtimer) rather
+		 * than restoring it completely, since SECONDS should
+		 * run in the calling function, too.
+		 */
+		if (PM_TYPE(pm->flags) == PM_INTEGER)
+		{
+		    pm->sets.ifn(pm, pm->gets.ifn(pm) + tpm->u.val);
+		}
+		else
+		{
+		    pm->sets.ffn(pm, pm->gets.ffn(pm) + tpm->u.dval);
+		}
+		tpm->flags |= PM_NORESTORE;
+	    }
 	    DPUTS(!tpm || PM_TYPE(pm->flags) != PM_TYPE(tpm->flags) ||
 		  !(tpm->flags & PM_SPECIAL),
 		  "BUG: in restoring scope of special parameter");

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: SECONDS can be floating point

[Wayne Davison]

On Tue, Oct 29, 2002 at 11:23:18AM +0000, Peter Stephenson wrote:
> This fixes the problem by restoring SECONDS to include the elapsed
> time spent in the function.  Of course, the accuracy won't be perfect,
> but it wouldn't be anyway.

How about this instead:  keep the value internally in float format
continuously, and then change back to a single accessor function that
returns a rounded integer if the type of the variable is set to integer.
This helps to avoid rounding errors, and keeps things a little more
accurate in this scenario.

..wayne..

Re: PATCH: SECONDS can be floating point

[Wayne Davison]

On Tue, Oct 29, 2002 at 11:23:18AM +0000, Peter Stephenson wrote:
> The problem was that if you made SECONDS local, then on return to the
> parent function, the time spent in the lower function didn't appear in
> the value of SECONDS

Are you sure this was a problem?  The internal value is a start time,
and the value returned is relative to "now", so it seems to me that as
long as the parent's SECONDS value is saved/restored literally when
localized that there is no need to fudge the value at all.  Perhaps the
bug is that the value is being saved/restored relative to "now"?  (I
haven't looked at the local-variable code enough to know for sure.)

..wayne..

Re: PATCH: SECONDS can be floating point

[Peter Stephenson]

Wayne Davison wrote:
> Perhaps the
> bug is that the value is being saved/restored relative to "now"?  (I
> haven't looked at the local-variable code enough to know for sure.)

Yes, that is what happens, so an alternative fix is to save and restore
what's in shtime rather than use the value reported by the parameter.
This requires yet more ugly parameter-specific code.  In particular, you
need to store a complete struct timeval.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

PATCH: my "SECONDS can be floating point" tweaks

[Wayne Davison]

On Mon, Oct 28, 2002 at 06:55:52PM +0000, Peter Stephenson wrote:
> This allows typeset to switch the SECONDS special parameter between
> integer and floating point

Here's a fairly simple patch that I think improves things a bit.

First, the intseconds{get,set}fn() functions are implemented by calling
the float* versions.  This makes the internal representation of the
value identical regardless of type (i.e. it ensures that shtimer.tv_usec
gets set relative to the current time, and that it is subtracted out on
get).

The second change may be more controversial.  I introduced two new
functions that allow us to get and set the "raw" (non-relative to now)
value of the SECONDS variable (though the value is returned as a
double).  I then added some code to builtin.c that saves this raw value
into the cached off u.dval, and changed the new code in params.c that
was adding local-elapsed time into the parent variable to just reload
the raw parent value.

A few other minor tweaks are also included, such as not testing the
newspecial enum against 0, enhancing the $SECONDS comment, and casting
the shtimer.* values to zlong instead of int (just in case int is too
small).

I won't commit this until I get some positive feedback about the
changes.

..wayne..

---8<------8<------8<------8<---cut here--->8------>8------>8------>8---
Index: Src/builtin.c
--- Src/builtin.c	29 Oct 2002 10:56:40 -0000	1.89
+++ Src/builtin.c	30 Oct 2002 21:08:28 -0000
@@ -1774,6 +1774,8 @@
 	    return NULL;
 	}
     }
+    else if (newspecial != NS_NONE && strcmp(pname, "SECONDS") == 0)
+	newspecial = NS_SECONDS;
 
     /*
      * A parameter will be local if
@@ -1913,8 +1915,12 @@
 	 * because we've checked for unpleasant surprises above.
 	 */
 	pm->flags = (PM_TYPE(pm->flags) | on | PM_SPECIAL) & ~off;
-	if (newspecial == NS_SECONDS)
+	if (newspecial == NS_SECONDS) {
+	    /* We save off the raw internal value of the SECONDS var */
+	    tpm->u.dval = getrawseconds();
 	    setsecondstype(pm, on, off);
+	}
+
 	/*
 	 * Final tweak: if we've turned on one of the flags with
 	 * numbers, we should use the appropriate integer.
@@ -1998,7 +2004,7 @@
 		  "BUG: parameter recreated with wrong flags");
 	    unsetparam_pm(ipm, 0, 1);
 	}
-    } else if (newspecial && !(pm->old->flags & PM_NORESTORE)) {
+    } else if (newspecial != NS_NONE && !(pm->old->flags & PM_NORESTORE)) {
 	/*
 	 * We need to use the special setting function to re-initialise
 	 * the special parameter to empty.
Index: Src/params.c
--- Src/params.c	29 Oct 2002 12:58:03 -0000	1.68
+++ Src/params.c	30 Oct 2002 21:08:29 -0000
@@ -97,7 +97,9 @@
 /**/
 unsigned char hatchar, hashchar;
  
-/* $SECONDS = time(NULL) - shtimer.tv_sec */
+/* $SECONDS = now.tv_sec - shtimer.tv_sec
+ *          + (now.tv_usec - shtimer.tv_usec) / 1000000.0
+ * (rounded to an integer if the parameter is not set to float) */
  
 /**/
 struct timeval shtimer;
@@ -2672,7 +2674,7 @@
 zlong
 intsecondsgetfn(Param pm)
 {
-    return time(NULL) - shtimer.tv_sec;
+    return (zlong)floatsecondsgetfn(pm);
 }
 
 /* Function to set value of special parameter `SECONDS' */
@@ -2681,8 +2683,7 @@
 void
 intsecondssetfn(Param pm, zlong x)
 {
-    shtimer.tv_sec = time(NULL) - x;
-    shtimer.tv_usec = 0;
+    floatsecondssetfn(pm, (double)x);
 }
 
 /**/
@@ -2706,8 +2707,23 @@
     struct timezone dummy_tz;
 
     gettimeofday(&now, &dummy_tz);
-    shtimer.tv_sec = now.tv_sec - (int)x;
-    shtimer.tv_usec = now.tv_usec - (int)((x - (double)(int)x) * 1000000.0);
+    shtimer.tv_sec = now.tv_sec - (zlong)x;
+    shtimer.tv_usec = now.tv_usec - (zlong)((x - (zlong)x) * 1000000.0);
+}
+
+/**/
+double
+getrawseconds(void)
+{
+    return (double)shtimer.tv_sec + (double)shtimer.tv_usec / 1000000.0;
+}
+
+/**/
+void
+setrawseconds(double x)
+{
+    shtimer.tv_sec = (zlong)x;
+    shtimer.tv_usec = (zlong)((x - (zlong)x) * 1000000.0);
 }
 
 /**/
@@ -3487,19 +3503,10 @@
 	    {
 		setsecondstype(pm, PM_TYPE(tpm->flags), PM_TYPE(pm->flags));
 		/*
-		 * We restore SECONDS by adding back in the elapsed
-		 * time (from the point we reset shtimer) rather
-		 * than restoring it completely, since SECONDS should
-		 * run in the calling function, too.
+		 * We restore SECONDS by restoring its raw internal value
+		 * that we cached off into tpm->u.dval.
 		 */
-		if (PM_TYPE(pm->flags) == PM_INTEGER)
-		{
-		    pm->sets.ifn(pm, pm->gets.ifn(pm) + tpm->u.val);
-		}
-		else
-		{
-		    pm->sets.ffn(pm, pm->gets.ffn(pm) + tpm->u.dval);
-		}
+		setrawseconds(tpm->u.dval);
 		tpm->flags |= PM_NORESTORE;
 	    }
 	    DPUTS(!tpm || PM_TYPE(pm->flags) != PM_TYPE(tpm->flags) ||
---8<------8<------8<------8<---cut here--->8------>8------>8------>8---

Re: PATCH: my "SECONDS can be floating point" tweaks

[Bart Schaefer]

On Oct 30,  1:17pm, Wayne Davison wrote:
} Subject: PATCH: my "SECONDS can be floating point" tweaks
}
} I won't commit this until I get some positive feedback about the
} changes.

I think they look good, except (maybe) for one thing:

} First, the intseconds{get,set}fn() functions are implemented by calling
} the float* versions.

Is converting to float and then back to integer (or the other way round
for the setfn) really going to give the same results as a computation
directly on only shtimer.tv_sec?

I wouldn't want to lose or gain an entire second due to rounding errors
when e.g. doing arithmetic in a loop on the integer SECONDS.

(Is "rounding" the right description anyway?  Isn't it _truncated_ to an
integer when the parameter is not set to float?)

I otherwise like the simplicity of this approach.

} The second change may be more controversial.  I introduced two new
} functions that allow us to get and set the "raw" (non-relative to now)
} value of the SECONDS variable (though the value is returned as a
} double).  I then added some code to builtin.c that saves this raw value
} into the cached off u.dval, and changed the new code in params.c that
} was adding local-elapsed time into the parent variable to just reload
} the raw parent value.

I must be sleepy or especially dense at the moment, but I don't follow
what this means from the point of view of the _caller_ of a function in
which SECONDS is local.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: my "SECONDS can be floating point" tweaks

[Peter Stephenson]

Wayne Davison wrote:
> Here's a fairly simple patch that I think improves things a bit.

Looks OK, although I still vaguely think a struct timeval is more
appropriate for the raw values (which doesn't fit inside struct param).
If you're convinced an overflow is unlikely, I don't really care,
though.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: my "SECONDS can be floating point" tweaks

[Wayne Davison]

On Thu, Oct 31, 2002 at 05:09:31AM +0000, Bart Schaefer wrote:
> Is converting to float and then back to integer (or the other way round
> for the setfn) really going to give the same results as a computation
> directly on only shtimer.tv_sec?

Not the same but more accurate.  For example:  if the script starts in
second "5" (long ago) .9 seconds into that second and the script
references $SECONDS .2 seconds later, the old version would report that
we've been running for 1 second.  The new version would report 0 seconds
have elapsed until we get .9 seconds into second "6".  So, the new
method causes the time to be calculated more accurately and then the
decimal points just get truncated (as you surmised: the value is not
rounded).

> I don't follow what this means from the point of view of the _caller_
> of a function in which SECONDS is local.

What it means is that the parent function's variable remains unaffected
by any child's local parameter.  Before this change the parent's value
could get more and more inaccurate each time a local value was used and
then added into the parent's value.  I.e., the old method was to save
off the elapsed time (say "8" seconds), run the local function, at which
point we add in the elapsed time of the child function (say "1") to our
value and then reset the SECONDS variable in the parent (setting it to
"9").  If we were near a second boundary in either the saving of the "8"
or the adding of the "1", the real value could easily be 1 second off
(i.e. "10"), but that should be a fairly rare error (I would imagine).
Switching the parent into float for better accuracy would (I believe)
cause a steady stream of micro-seconds errors from each child to
accumulate at a very slow rate (since the time used in the code that
saves the parent value and adds the child value contains untimed
sections).

The new code just remembers the real start time of the parent's variable
and restores it, so the value remains unchanged no matter how many calls
are made.

..wayne..

Re: PATCH: my "SECONDS can be floating point" tweaks

[Wayne Davison]

On Thu, Oct 31, 2002 at 10:34:45AM +0000, Peter Stephenson wrote:
> Looks OK, although I still vaguely think a struct timeval is more
> appropriate for the raw values (which doesn't fit inside struct param).

That is certainly an option, and it would be easy to tweak my code to
implement this if it is desired (after adding a "struct timeval" to the
param.u union).

In my limited testing the sub-second value was restored exactly, but I
can imagine that certain fractional representations would cause the
sub-second value to get tweaked by a small amount the first time it was
saved and restored (and remain constant after that).  I haven't checked
yet to know for sure how big this tweak could get, but I'll look at it
more closely soon.  (My gut says that the values will be close enough
that the change will not be noticed.)

I've gone ahead and committed my previous patch.

..wayne..