[PATCH weston 1/8] shared: Add timespec_normalize helper

[Alexandros Frantzis]

On Tue, Dec 12, 2017 at 12:55:27PM +0200, Pekka Paalanen wrote:
> On Tue, 12 Dec 2017 12:36:39 +0200
> Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:
> 
> > On Tue, Dec 12, 2017 at 11:50:49AM +0200, Pekka Paalanen wrote:
> > > On Mon,  4 Dec 2017 15:34:01 +0200
> > > Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:
> > >   
> > > > Add a helper function to normalize struct timespec values so that the
> > > > nanoseconds part is less than 1 second and has the same sign as the
> > > > seconds part (if the seconds part is not 0).
> > > > 
> > > > Normalization is required to ensure we can safely convert timespec
> > > > values to wayland protocol data, i.e, to tv_sec_hi, tv_sec_lo,
> > > > tv_sec_nsec triplets, and will be used in upcoming commits.
> > > > 
> > > > Signed-off-by: Alexandros Frantzis <alexandros.frantzis at collabora.com>
> > > > ---
> > > >  shared/timespec-util.h | 28 ++++++++++++++++++++++
> > > >  tests/timespec-test.c  | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 93 insertions(+)
> > > > 
> > > > diff --git a/shared/timespec-util.h b/shared/timespec-util.h
> > > > index f9736c27..a10edf5b 100644
> > > > --- a/shared/timespec-util.h
> > > > +++ b/shared/timespec-util.h
> > > > @@ -33,6 +33,34 @@
> > > >  
> > > >  #define NSEC_PER_SEC 1000000000
> > > >  
> > > > +/* Normalize a timespec
> > > > + *
> > > > + * \param r[out] normalized timespec
> > > > + * \param a[in] timespec to normalize
> > > > + *
> > > > + * Normalize a timespec so that tv_nsec is less than 1 second
> > > > + * and has the same sign as tv_sec (if tv_sec is non-zero).  
> > > 
> > > Hi,  
> > 
> > Hi Pekka,
> > 
> > thanks for the review.
> > 
> > > why does it need to have the same sign?
> > > E.g. timespec_sub() ensures nsec is non-negative, as do the add
> > > functions.  
> > 
> > The goal was to make timespec_normalize more generally useful and
> > independent of any current behavior. I didn't want to assume that the
> > provided timespec would be produced necessarily by using the
> > aforementioned functions that already provide the same-sign guarantee.
> 
> They don't provide the same-sign guarantee. They ensure nsec is
> non-negative, regardless of the sign of sec.

Ah, I misunderstood.

> The question here is, what is the normalized form?
>
> We have not had use for negative time values in the protocol, and the
> new proposal does not either, so protocol specs do not offer any
> precendent. Computationally they could appear in programs, so the
> definition of "normalized" for these helper functions will be purely
> libweston-internal.
> 
> I'd go with non-negative instead of same-sign, because the existing
> code is already like that. I'd like to hear about any better
> justification one way or another, since I have little else. I believe
> non-negative lead to simpler code.

The main justification for same sign is a more intuitive representation
for humans. That is, it's more natural to think of -1.5 as -1 + -0.5,
rather than -2 + 0.5.

However, that's not a problem from the computer's point of view and I
had missed the pre-existing non-negative nsec guarantee in other
functions.  Also, since in the normalized form nsec < 1 sec, checking
the sign of tv_sec is enough to determine the sign of the timespec which
is a nice property to maintain.

So, bottom line, I am convinced. I will change timespec_normalize to
provide the non-negative nsec guarantee.

Thanks,
Alexandros