file-newer-than-file-p [was: [COMMIT] Check absolute source file ...]

[Stephen J. Turnbull]

Continued from xemacs-patches.

(BTW congratulations on your belated Xmas present: a trivially fixable
bug.  Obvious in retrospect but I didn't see it at all. :-)

Aidan Kehoe writes:

 > OK, fixed--the API of #'file-new-than-file-p is stupidly unintuitive, I
 > smell the work of a MacArthur genius. Thanks for the report!

What would you suggest for the API?  An error in the case where the
either or both of the files doesn't exist?  An error in the case where
the "baseline" file doesn't exist?  A change in the value returned?

Let the API be `(file-newer-than-file-p FILE BASELINE)', and in a
make-like application, nil => file needs to be made.  For sure I would
want:

(1) If FILE and BASELINE both exist, return t if FILE is dated
    (strictly) more recently than BASELINE, else return nil.

(2) If FILE does not exist but BASELINE does, return nil.

I think I would also want:

(3) If neither FILE nor BASELINE exists, return nil.

It's arguable that the anti-symmetry of a total order implies that
from (2) we want:

(4) If FILE exists but BASELINE does not, return t.

The docstring says

    Return t if file FILE1 is newer than file FILE2.
    If FILE1 does not exist, the answer is nil;
    otherwise, if FILE2 does not exist, the answer is t.

which is precisely what (1) ... (4) give.  Do you disagree, or do you
just think the whole idea of trying to extend the relation to
non-existent files is bogus and the caller should check explicitly?

 >  	    (if (and (null docfile-out-of-date)
 > -		     (file-newer-than-file-p arg docfile))
 > +                     ;; We need to check the absolute path here:
 > +		     (file-newer-than-file-p absolute docfile))
 >  		(setq docfile-out-of-date t))

I would argue that the above code snippet (from your patch to
make-docfile.el) has the arguments in the wrong order.  (That's
historical, not your change, of course.)  The logic should be

    (unless (file-newer-than-file-p docfile arg)
      (setq docfile-out-of-date))

(where of course the check for `docfile-out-of-date' is an
optimization).  Of course, had you rewritten the patch this way, the
make wouldn't have failed, and lots of us would have ended up with
more and more out-of-date DOC files!

Speaking of optimizations, how about rewriting that section of code
this way:

(setq docfile-out-of-date
      (catch 'out-of-date
        ;; mapc is in cl, mapcar is a builtin.
        (mapcar (lambda (library)
                  (let (absolute)
                     ;; order matters
                     ;; #### why do we use .elc here?
                     ;; Even if we want to build DOC with .elcs,
                     ;; shouldn't the check be on the .el?
                     (setq library  (packages-add-suffix library))
                           absolute (locate-library library))
                     (when (null absolute)
                       ;; Message was "Error: ..." but I don't see a
                       ;; need to stop the build for this.
                       (message "Warning: dumped file %s does not exist"
                                absolute)
                       ;; Uncommentable comments go here :-}
                       ;; Mike, can we remove them?
                       )
                     (unless (file-newer-than-file-p docfile absolute)
                       (throw 'out-of-date t))))
                 preloaded-file-list)
        nil)

BTW, do you agree with me (at #### above) that we want to use the .el
file, not the .elc?