Re: [Emacs-diffs] master 8e2b2a2: Minor cleanup in pdumper.c

Re: [Emacs-diffs] master 8e2b2a2: Minor cleanup in pdumper.c

[Daniel Colascione]

> branch: master
> commit 8e2b2a2b179c3ed170ad9de32a320e788c6a3a5e
> Author: Eli Zaretskii <eliz@gnu.org>
> Commit: Eli Zaretskii <eliz@gnu.org>
>
>     Minor cleanup in pdumper.c
>
>     * src/pdumper (subtract_timespec): Function removed.
>     (pdumper_load): Use timespec_sub instead of subtract_timespec.
> ---
>  src/pdumper.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/src/pdumper.c b/src/pdumper.c
> index b51a379..1c49167 100644
> --- a/src/pdumper.c
> +++ b/src/pdumper.c
> @@ -5388,15 +5388,6 @@ enum dump_section
>     NUMBER_DUMP_SECTIONS,
>    };
>
> -/* Subtract two timespecs, yielding a difference in milliseconds. */
> -static double
> -subtract_timespec (struct timespec minuend, struct timespec subtrahend)

Cmon, don't you think we should do better than open-code timespec operations?

Re: [Emacs-diffs] master 8e2b2a2: Minor cleanup in pdumper.c

[Daniel Colascione]

>> branch: master
>> commit 8e2b2a2b179c3ed170ad9de32a320e788c6a3a5e
>> Author: Eli Zaretskii <eliz@gnu.org>
>> Commit: Eli Zaretskii <eliz@gnu.org>
>>
>>     Minor cleanup in pdumper.c
>>
>>     * src/pdumper (subtract_timespec): Function removed.
>>     (pdumper_load): Use timespec_sub instead of subtract_timespec.
>> ---
>>  src/pdumper.c | 16 +++++-----------
>>  1 file changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/pdumper.c b/src/pdumper.c
>> index b51a379..1c49167 100644
>> --- a/src/pdumper.c
>> +++ b/src/pdumper.c
>> @@ -5388,15 +5388,6 @@ enum dump_section
>>     NUMBER_DUMP_SECTIONS,
>>    };
>>
>> -/* Subtract two timespecs, yielding a difference in milliseconds. */
>> -static double
>> -subtract_timespec (struct timespec minuend, struct timespec subtrahend)
>
> Cmon, don't you think we should do better than open-code timespec
> operations?

Ah, never mind. I saw the constants at the end of the patch and jumped the
gun.

Re: [Emacs-diffs] master 8e2b2a2: Minor cleanup in pdumper.c

[Paul Eggert]

Daniel Colascione wrote:
> Ah, never mind. I saw the constants at the end of the patch and jumped the
> gun.

While we're on the subject, why does pdumper-stats return milliseconds? 
Returning seconds would be simpler, and the number is a float so I don't see the 
point of picking some other unit of measure.

Re: [Emacs-diffs] master 8e2b2a2: Minor cleanup in pdumper.c

[Daniel Colascione]

> Daniel Colascione wrote:
>> Ah, never mind. I saw the constants at the end of the patch and jumped
>> the
>> gun.
>
> While we're on the subject, why does pdumper-stats return milliseconds?
> Returning seconds would be simpler, and the number is a float so I don't
> see the
> point of picking some other unit of measure.

No reason in particular. We can always change it.

Re: [Emacs-diffs] master 8e2b2a2: Minor cleanup in pdumper.c

[Paul Eggert]

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

Daniel Colascione wrote:
> No reason in particular. We can always change it.

OK, thanks, I did that by installing the attached. (One of my pet little peeves 
is avoiding rounding errors, admittedly overkill here.)

[-- Attachment #2: 0001-pdumper-stats-now-returns-s-not-ms.patch --]
[-- Type: text/x-patch, Size: 2196 bytes --]

From 7b82c1efa10b249191be51a9bc001a345b26ce2e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 19 Jan 2019 14:38:52 -0800
Subject: [PATCH] pdumper-stats now returns s, not ms

* doc/lispref/internals.texi (pdumper-stats):
* src/pdumper.c (pdumper_load): Return seconds, not milliseconds.
Minimize rounding errors in the usual case.
---
 doc/lispref/internals.texi | 2 +-
 src/pdumper.c              | 9 +++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/doc/lispref/internals.texi b/doc/lispref/internals.texi
index 437657f243..06ff9f70bf 100644
--- a/doc/lispref/internals.texi
+++ b/doc/lispref/internals.texi
@@ -232,7 +232,7 @@ Building Emacs
 @w{@code{((dumped-with-pdumper . t) (load-time . @var{time})
 (dump-file-name . @var{file}))}},
 where @var{file} is the name of the dump file, and @var{time} is the
-time in milliseconds it took to restore the state from the dump file.
+time in seconds it took to restore the state from the dump file.
 If the current session was not restored from a portable dump file, the
 value is nil.
 @end defun
diff --git a/src/pdumper.c b/src/pdumper.c
index 19a21329b1..4bbeabb828 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -5545,9 +5545,10 @@ pdumper_load (const char *dump_filename)
 
   struct timespec load_timespec =
     timespec_sub (current_timespec (), start_time);
-  dump_private.load_time =
-    (double) load_timespec.tv_sec * 1000.0
-    + (double) load_timespec.tv_nsec * 0.000001;
+  ALLOW_IMPLICIT_CONVERSION;
+  double s = load_timespec.tv_sec, ns = load_timespec.tv_nsec;
+  DISALLOW_IMPLICIT_CONVERSION;
+  dump_private.load_time = (s * 1e9 + ns) / 1e9;
   dump_private.dump_filename = dump_filename_copy;
   dump_filename_copy = NULL;
 
@@ -5569,7 +5570,7 @@ the return value is an alist of the form:
 
   ((dumped-with-pdumper . t) (load-time . TIME) (dump-file-name . FILE))
 
-where TIME is the time in milliseconds it took to restore Emacs state
+where TIME is the time in seconds it took to restore Emacs state
 from the dump file, and FILE is the name of the dump file.
 Value is nil if this session was not started using a portable dump file.*/)
      (void)
-- 
2.17.1

Re: [Emacs-diffs] master 8e2b2a2: Minor cleanup in pdumper.c

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 19 Jan 2019 14:41:04 -0800
> Cc: emacs-devel@gnu.org
> 
> diff --git a/src/pdumper.c b/src/pdumper.c
> index 19a21329b1..4bbeabb828 100644
> --- a/src/pdumper.c
> +++ b/src/pdumper.c
> @@ -5545,9 +5545,10 @@ pdumper_load (const char *dump_filename)
>  
>    struct timespec load_timespec =
>      timespec_sub (current_timespec (), start_time);
> -  dump_private.load_time =
> -    (double) load_timespec.tv_sec * 1000.0
> -    + (double) load_timespec.tv_nsec * 0.000001;
> +  ALLOW_IMPLICIT_CONVERSION;
> +  double s = load_timespec.tv_sec, ns = load_timespec.tv_nsec;
> +  DISALLOW_IMPLICIT_CONVERSION;
> +  dump_private.load_time = (s * 1e9 + ns) / 1e9;

Any reason not to use timespectod?

Re: [Emacs-diffs] master 8e2b2a2: Minor cleanup in pdumper.c

[Paul Eggert]

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

Eli Zaretskii wrote:
> Any reason not to use timespectod?

Thanks, I had forgotten about timespectod. I installed the attached. Yay, one 
less use of ALLOW_IMPLICIT_CONVERSION, a macro I'm not a fan of.

[-- Attachment #2: 0001-Simplify-pdumper-load-via-timespectod.patch --]
[-- Type: text/x-patch, Size: 1016 bytes --]

From be73ed4338b37973eb67e8fd91aaa9e1166c665c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 21 Jan 2019 12:48:42 -0800
Subject: [PATCH] Simplify pdumper-load via timespectod

Suggested by Eli Zaretskii in:
https://lists.gnu.org/r/emacs-devel/2019-01/msg00458.html
* src/pdumper.c (pdumper_load): Simplify.
---
 src/pdumper.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/pdumper.c b/src/pdumper.c
index e57aa8f2a0..6be26dc816 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -5545,10 +5545,7 @@ pdumper_load (const char *dump_filename)
 
   struct timespec load_timespec =
     timespec_sub (current_timespec (), start_time);
-  ALLOW_IMPLICIT_CONVERSION;
-  double s = load_timespec.tv_sec, ns = load_timespec.tv_nsec;
-  DISALLOW_IMPLICIT_CONVERSION;
-  dump_private.load_time = (s * 1e9 + ns) / 1e9;
+  dump_private.load_time = timespectod (load_timespec);
   dump_private.dump_filename = dump_filename_copy;
   dump_filename_copy = NULL;
 
-- 
2.17.1

Re: [Emacs-diffs] master 8e2b2a2: Minor cleanup in pdumper.c

[Daniel Colascione]

On 1/21/19 3:49 PM, Paul Eggert wrote:
> Eli Zaretskii wrote:
>> Any reason not to use timespectod?
> 
> Thanks, I had forgotten about timespectod. I installed the attached. 
> Yay, one less use of ALLOW_IMPLICIT_CONVERSION, a macro I'm not a fan of.

It beats trying to debug silent integral truncation problems without 
compiler help. IMHO, it was a mistake for C to allow silent destructive 
narrowing conversion.

Re: [Emacs-diffs] master 8e2b2a2: Minor cleanup in pdumper.c

[Paul Eggert]

Daniel Colascione wrote:
>> Thanks, I had forgotten about timespectod. I installed the attached. Yay, one 
>> less use of ALLOW_IMPLICIT_CONVERSION, a macro I'm not a fan of.
> 
> It beats trying to debug silent integral truncation problems without compiler 
> help. IMHO, it was a mistake for C to allow silent destructive narrowing 
> conversion.

In my experience the warning is more trouble than it's worth in Emacs source 
code, as its signal-to-noise ratio is too large there. There are no free lunches 
in software development, and this particular lunch goes waaayyy over budget.

Although I also would like to go back to 1975 and whisper advice in Thompson & 
Ritchie's ears, it's a bit late for that now. Besides, they would have disagreed 
with me anyway, and with you too. (Some years later I argued with Ritchie on the 
topic of C's integer widths and promotions, and failed to convince him.)

Re: [Emacs-diffs] master 8e2b2a2: Minor cleanup in pdumper.c

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 21 Jan 2019 23:24:29 -0800
> 
> Daniel Colascione wrote:
> >> Thanks, I had forgotten about timespectod. I installed the attached. Yay, one 
> >> less use of ALLOW_IMPLICIT_CONVERSION, a macro I'm not a fan of.
> > 
> > It beats trying to debug silent integral truncation problems without compiler 
> > help. IMHO, it was a mistake for C to allow silent destructive narrowing 
> > conversion.
> 
> In my experience the warning is more trouble than it's worth in Emacs source 
> code, as its signal-to-noise ratio is too large there. There are no free lunches 
> in software development, and this particular lunch goes waaayyy over budget.

The C language is for people who know what they are doing.  It would
be IMO unacceptable for it to reject assignments of a double value to
an int, or even warn by default about it, because any serious
numerical program does that all the time.

Re: [Emacs-diffs] master 8e2b2a2: Minor cleanup in pdumper.c

[Daniel Colascione]

On 1/22/19 8:53 AM, Eli Zaretskii wrote:
>> Cc: emacs-devel@gnu.org
>> From: Paul Eggert <eggert@cs.ucla.edu>
>> Date: Mon, 21 Jan 2019 23:24:29 -0800
>>
>> Daniel Colascione wrote:
>>>> Thanks, I had forgotten about timespectod. I installed the attached. Yay, one
>>>> less use of ALLOW_IMPLICIT_CONVERSION, a macro I'm not a fan of.
>>>
>>> It beats trying to debug silent integral truncation problems without compiler
>>> help. IMHO, it was a mistake for C to allow silent destructive narrowing
>>> conversion.
>>
>> In my experience the warning is more trouble than it's worth in Emacs source
>> code, as its signal-to-noise ratio is too large there. There are no free lunches
>> in software development, and this particular lunch goes waaayyy over budget.
> 
> The C language is for people who know what they are doing.  It would
> be IMO unacceptable for it to reject assignments of a double value to
> an int, or even warn by default about it, because any serious
> numerical program does that all the time.

The C language is for people who know what they are doing.  It would be 
IMO unacceptable for it to reject assignments of a char* to a struct foo 
*, or even warn by default about it, because any serious systems program 
does that all the time.

Re: [Emacs-diffs] master 8e2b2a2: Minor cleanup in pdumper.c

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > The C language is for people who know what they are doing.  It would be 
  > IMO unacceptable for it to reject assignments of a char* to a struct foo 
  > *, or even warn by default about it, because any serious systems program 
  > does that all the time.

I get the impression that this is meant ironically, as part of an
elliptical argument against the point someone else made about assigning a
double value to an int variable.  Was that what you meant?
You did not say that, and I think many people on the list will not
figure that out.

That argument depends on background about the rules of C which was not
stated.  Many will not know that background,

For clear communication, please always spell out the steps in your
argument.  To present just one step, and expect people to figure out
how it relates to the context, is a recipe for incomprehension and
confusion.

(Why use emacs-devel to critique the rules of C?)

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)