Re: [Emacs-diffs] trunk r115596: Minor fixes for recent openp changes.

Re: [Emacs-diffs] trunk r115596: Minor fixes for recent openp changes.

[Jarek Czekalski]

I have problems with understanding the new code. Especially the 
TYPE_MINIMUM part. Paul, maybe you could add a comment to explain why 
such a value must be put here? Could this be initialized to 0 or to 
invalid_timespec? It would be easier to understand.

When the save_mtime is invalid (nsec < 0), the result of its comparison 
could be undefined. Maybe this comparison never happens, however 0 value 
would be safer.

I have these 2 doubts. Maybe the problem comes from lack of knowledge on 
my side. I don't know :) Just sharing thoughts.

Thanks
Jarek

-  struct timespec save_mtime;
-  int save_fd = 0;
+  struct timespec save_mtime = make_timespec (TYPE_MINIMUM (time_t), -1);
+  int save_fd = -1;

-                      if (!save_fd || timespec_cmp (save_mtime, mtime) < 0)
+              if (timespec_cmp (mtime, save_mtime) <= 0)

Re: [Emacs-diffs] trunk r115596: Minor fixes for recent openp changes.

[Paul Eggert]

Jarek Czekalski wrote:
> maybe you could add a comment

Sure, done in trunk bzr 115598.

> Could this be initialized to 0 or to invalid_timespec?

No, because valid time stamps can be negative, and neither
0 nor invalid_timestamp would compare less than these
valid time stamps.

Here's a shell transcript to illustrate:

$ touch -d 0001-12-25 Christmas-1-AD
$ ls -l oldfile
-rw-r--r--. 1 eggert eggert 0 Dec 25  0001 Christmas-1-AD
$ ls -l --time-style=+%s oldfile
-rw-r--r--. 1 eggert eggert 0 -62104665600 Christmas-1-AD

For this file, file-attributes returns the time stamp
(-947642 512 0 0), and -62104665600 == (-947642 << 16) + 512.

Re: [Emacs-diffs] trunk r115596: Minor fixes for recent openp changes.

[Jarek Czekalski]

Paul,

Thanks for the explanations and the comment. Some things get cleared. 
The intent is that we are able to deal with files left on a harddisk 
from Middle Ages, that's cool :)

src/lread.c:
   /* The last-modified time of the newest matching file found.
      Initialize it to something less than all valid timestamps.  */
   struct timespec save_mtime = make_timespec (TYPE_MINIMUM (time_t), -1);

But -1 makes this struct invalid. How can we be sure that comparing an 
invalid timestamp to a valid timestamp gives us the expected result?

src/systime.h:
INLINE bool
timespec_valid_p (struct timespec t)
{
   return t.tv_nsec >= 0;
}

So after having this shallow look at timespec structure, I think -1 
should be replaced by 0. 999999999, even if more correct, would probably 
be too pedantic and less readable.

Jarek

W dniu 2013-12-18 23:45, Paul Eggert pisze:
> [...]
> Here's a shell transcript to illustrate:
>
> $ touch -d 0001-12-25 Christmas-1-AD
> $ ls -l oldfile
> -rw-r--r--. 1 eggert eggert 0 Dec 25  0001 Christmas-1-AD
> $ ls -l --time-style=+%s oldfile
> -rw-r--r--. 1 eggert eggert 0 -62104665600 Christmas-1-AD
>
> For this file, file-attributes returns the time stamp
> (-947642 512 0 0), and -62104665600 == (-947642 << 16) + 512.
>
>

Re: [Emacs-diffs] trunk r115596: Minor fixes for recent openp changes.

[Eli Zaretskii]

> Date: Thu, 19 Dec 2013 08:18:24 +0100
> From: Jarek Czekalski <jarekczek@poczta.onet.pl>
> 
> src/lread.c:
>    /* The last-modified time of the newest matching file found.
>       Initialize it to something less than all valid timestamps.  */
>    struct timespec save_mtime = make_timespec (TYPE_MINIMUM (time_t), -1);
> 
> But -1 makes this struct invalid. How can we be sure that comparing an 
> invalid timestamp to a valid timestamp gives us the expected result?

Look at timespec_cmp, and you will know.

Re: [Emacs-diffs] trunk r115596: Minor fixes for recent openp changes.

[Jarek Czekalski]

W dniu 2013-12-19 18:00, Eli Zaretskii pisze:
> Look at timespec_cmp, and you will know. 

It's in lib directory. I lost hope to find it when I found src/systime.h 
and no timespec_cmp there. I thought it was a system dependent function. 
Now I know that some invalid values may be used in comparisons. If they 
were accessible through something like min_timespec and max_timespec, 
this thread would not exist.

Anyway, sorry for the noise.

Jarek