Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update internal to avoid precision destruction #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

psionic-k
Copy link

First things first regarding #25

I noticed while writing a test that #'ts-apply and `#'ts-update' were mutating the unix time even though it's unnecessary.

A proper fix started to rabbit hole a bit:

  • If calling ts-apply with just a :unix time, there's no need to update the unix time again in ts-update. Every other slot could update if non-nil, but the unix time should not
  • There is no error when calls are made with both unix or internal and any combination of day, month etc, but these calls are surely an improper usage

The kinds of tests I was running and expecting to work:

(let* ((time (float-time))
                 (ts (ts-apply :unix time (ts-now))))
     `(,(ts-internal ts)
       ,(time-convert time t)))
;;  ((7133786877722624 . 4194304)
;;   (7133786879364915 . 4194304))

ts-update is not taking the easy path...

Here's another test

(let* ((time (time-convert (current-time) t))
                 (ts (ts-apply :internal time (ts-now))))
     `(,(ts-unix ts)
       ,(float-time time)))
     
(1700827572.0 1700827572.2982888)

Ah, so both times are actually truncating to seconds unnecessarily. I can expand the scope of the PR if you think it's likely to pay off.

Copy link
Owner

@alphapapa alphapapa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

test/test.el Outdated
Comment on lines 83 to 84
(should (and (consp (ts-internal (ts-now)))
(not (proper-list-p (ts-internal (ts-now)))))))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to test this, we should probably ensure that each element is an integer, as intended. I'd probably use pcase.

ts.el Outdated
@@ -222,7 +222,7 @@ slot `year' and alias `y' would create an alias `ts-y')."
;; MAYBE: Add tz-offset-minutes

(internal
nil :accessor-init (apply #'encode-time (decode-time (ts-unix struct))))
nil :accessor-init (funcall #'time-convert (ts-unix struct) t))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use funcall instead of calling time-convert directly?

@psionic-k
Copy link
Author

psionic-k commented Nov 25, 2023

Unless I missed something decode-time always loses precision. Even if every slot is populated, they could destroy precision when used to update the unix and or internal time.

Minor aside, I couldn't find a way to use fset with (decoded-time-year (decode-time)), which would make it much more obvious when doing mutations on those time structures. Never mind. setf all good. 🤡

I don't quite understand the README selling point about string-to-number. What's wrong with decoded-time-year?

@alphapapa
Copy link
Owner

alphapapa commented Nov 25, 2023

Unless I missed something decode-time always loses precision. Even if every slot is populated, they could destroy precision when used to update the unix and or internal time.

Ok, I'm taking your word on that for now.

I don't quite understand the README selling point about string-to-number. What's wrong with decoded-time-year?

This package predates that and similar functions. If those functions had existed, I might not have written this library, or it might have been written as a thin wrapper around them.

Nevertheless, I suspect this package may still have a performance advantage when only one or a few slots are needed, because it avoids decoding all slots. But I don't have time to re-benchmark the whole package against the latest date/time functions in current Emacs now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants