On Sun, 8 Jan 2017, J William Piggott wrote:
> > @@ -655,7 +655,8 @@ display_time(const bool hclock_valid, struct timeval hwctime)
> > * If anything goes wrong (and many things can), we return return code 10
> > * and arbitrary *time_p. Otherwise, return code is 0 and *time_p is valid.
> > */
> > -static int interpret_date_string(const struct hwclock_control *ctl, time_t * const time_p)
> > +static int interpret_date_string(const struct hwclock_control *ctl,
> > + time_t *const time_p)
>
>
> Sorry for nitpicking. I know you didn't write this, but as long as
> you're making a change here, shouldn't it be: const time_t *time_p?
Nope. That will make pointer value read-only, and that would cause
following error.
sys-utils/hwclock.c:720:11: error: assignment of read-only location '*time_p'
*time_p = seconds_since_epoch;
Code we have makes the pointer read-only, but leaves value writeable.
> > @@ -1031,9 +1032,7 @@ calculate_adjustment(const struct hwclock_control *ctl,
> > exact_adjustment =
> > ((double)(systime - last_time)) * factor / (24 * 60 * 60)
> > + not_adjusted;
> > - tdrift_p->tv_sec = (int) exact_adjustment;
> > - if (exact_adjustment < 0)
> > - tdrift_p->tv_sec--;
> > + tdrift_p->tv_sec = (int) floor(exact_adjustment);
>
> Is the cast required now?
floor(3) will return double, tv_sec is time_t. Yes I should and have
fixed the cast from int to time_t.
> >> @@ -158,28 +158,31 @@ static int do_rtc_read_ioctl(int rtc_fd, struct tm *tm)
> >> {
> >> int rc = -1;
> >> char *ioctlname;
> >> -
> >> #ifdef __sparc__
> >> /* some but not all sparcs use a different ioctl and struct */
> >> struct sparc_rtc_time stm;
> >> +#endif
> >>
> >> 8< -------
> >>
> >>>
> >>> Can't this be in the sparc ifdef just after this?
> >>>
> >>> Otherwise looks OK, thank you.
> >>>
> >
> > That would mix declaration and code. Kept as is.
>
> You mean keeping declarations at the top, I see. We break that rule
> sometimes. I think the code would look nicer in this case. But I don't
> have a strong opinion about it. It's fine your way.
Since C99 it is allowed to mix declarations and code, but quite franky I
find it irritating when declarations are hidden somewhere halfway down
function. Same story with #defines they should be at top of the file after
#includes before any functions - and no where else. IMHO these sort of
conventios make code reading, and understanding, easier.
-- snip
There is now new commit that removes --compare option, and all related
stuff. Naturally the earlier integer arithmetics change and deprecation in
manual page are removed from changes.
https://github.com/kerolasa/lelux-utiliteetit/commit/d3e33557e6794446c712ca264e54fe4b186a17ca
----------------------------------------------------------------
The following changes since commit 89958178f6d6ebe0944d423feaea66be521fff43:
If mtab support is disabled, disable ro/rw mtab checks (2017-01-10 17:32:32 +0100)
are available in the git repository at:
git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
for you to fetch changes up to d3e33557e6794446c712ca264e54fe4b186a17ca:
hwclock: remove --compare option (2017-01-11 21:04:36 +0000)
----------------------------------------------------------------
Thank you JWP for reviews, I hope I did not miss anything but if I did let
me know and I will follow up. Even better if everyone agrees that this is
now good enough to be merged, and we can move forward with other updates
such as adjtimex(8) work.
--
Sami Kerola
http://www.iki.fi/kerolasa/
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
