On Tue, 14 Feb 2017 13:18:01 +0000 Daniel Stone <dani...@collabora.com> wrote:
> Add a (timespec) = (timespec) + (nsec) helper, to save intermediate
> conversions to nanoseconds in its users.
>
> Signed-off-by: Daniel Stone <dani...@collabora.com>
Hi,
I recall some comments about using timespec over int64_t nanoseconds
being over-engineering, what do you think? Should we rather just
convert everything to int64_t nsec from the start and only deal with
them?
Personally I'm favour of timespec, even though int64_t nsec range is
+/- 290 years or so. :-)
> ---
> Makefile.am | 10 ++++
> shared/timespec-util.h | 21 +++++++++
> tests/timespec-test.c | 126
> +++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 157 insertions(+)
> create mode 100644 tests/timespec-test.c
>
> diff --git a/Makefile.am b/Makefile.am
> index dbdeea299..519d9115c 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1194,6 +1194,7 @@ internal_tests = \
>
> shared_tests = \
> config-parser.test \
> + timespec.test \
> string.test \
> vertex-clip.test \
> zuctest
> @@ -1307,6 +1308,15 @@ config_parser_test_CFLAGS = \
> $(AM_CFLAGS) \
> -I$(top_srcdir)/tools/zunitc/inc
>
> +timespec_test_SOURCES = tests/timespec-test.c
> +timespec_test_LDADD = \
> + libshared.la \
> + libzunitc.la \
> + libzunitcmain.la
> +timespec_test_CFLAGS = \
> + $(AM_CFLAGS) \
> + -I$(top_srcdir)/tools/zunitc/inc
> +
Ooh, a zuc test!
> string_test_SOURCES = \
> tests/string-test.c \
> shared/string-helpers.h
> diff --git a/shared/timespec-util.h b/shared/timespec-util.h
> index edd4ec143..80b557859 100644
> --- a/shared/timespec-util.h
> +++ b/shared/timespec-util.h
> @@ -49,6 +49,27 @@ timespec_sub(struct timespec *r,
> }
> }
>
> +/* Add a nanosecond value to a timespec
> + *
> + * \param r[out] result: a + b
> + * \param a[in] base operand as timespec
> + * \param b[in] operand in nanoseconds
> + */
> +static inline void
> +timespec_add_nsec(struct timespec *r, const struct timespec *a, int64_t b)
> +{
> + r->tv_sec = a->tv_sec + (b / NSEC_PER_SEC);
> + r->tv_nsec = a->tv_nsec + (b % NSEC_PER_SEC);
> +
> + if (r->tv_nsec >= NSEC_PER_SEC) {
> + r->tv_sec++;
> + r->tv_nsec -= NSEC_PER_SEC;
> + } else if (r->tv_nsec <= -NSEC_PER_SEC) {
IIRC tv_nsec cannot be negative, so it should be:
r->tv_nsec < 0
I recall writing asserts for tv_nsec, but maybe it wasn't weston where
I did that.
I realize mandating 0 <= nsec < NSEC_PER_SEC to be somewhat arbitrary,
but timespec_sub() already assumes that and it is what
presentation-time protocol requires.
In wesgr where I needed the notion of invalid timestamps, I abused the
value tv_nsec = -1 for it.
> + r->tv_sec--;
> + r->tv_nsec += NSEC_PER_SEC;
> + }
> +}
Otherwise it looks correct.
> +
> /* Convert timespec to nanoseconds
> *
> * \param a timespec
> diff --git a/tests/timespec-test.c b/tests/timespec-test.c
> new file mode 100644
> index 000000000..f1193507a
> --- /dev/null
> +++ b/tests/timespec-test.c
> @@ -0,0 +1,126 @@
> +/*
> + * Copyright © 2016 Collabora, Ltd.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining
> + * a copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sublicense, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial
> + * portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#include "config.h"
> +
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <assert.h>
> +#include <errno.h>
> +#include <unistd.h>
> +
> +#include "timespec-util.h"
> +
> +#include "shared/helpers.h"
> +#include "zunitc/zunitc.h"
> +
> +#define NSEC_PER_SEC 1000000000
NSEC_PER_SEC should come from timespec-util.h.
> +
> +ZUC_TEST(timespec_test, timespec_sub)
> +{
> + struct timespec a, b, r;
> +
> + a.tv_sec = 1;
> + a.tv_nsec = 1;
> + b.tv_sec = 0;
> + b.tv_nsec = 2;
> + timespec_sub(&r, &a, &b);
> + ZUC_ASSERT_EQ(r.tv_sec, 0);
> + ZUC_ASSERT_EQ(r.tv_nsec, NSEC_PER_SEC - 1);
> +}
> +
> +ZUC_TEST(timespec_test, timespec_to_nsec)
> +{
> + struct timespec a;
> +
> + a.tv_sec = 4;
> + a.tv_nsec = 4;
> + ZUC_ASSERT_EQ(timespec_to_nsec(&a), (NSEC_PER_SEC * 4ULL) + 4);
> +}
> +
> +ZUC_TEST(timespec_test, millihz_to_nsec)
> +{
> + ZUC_ASSERT_EQ(millihz_to_nsec(60000), 16666666);
> +}
> +
> +ZUC_TEST(timespec_test, timespec_add_nsec)
> +{
> + struct timespec a, r;
An array and a loop to ease readability? :-)
> +
> + a.tv_sec = 0;
> + a.tv_nsec = NSEC_PER_SEC - 1;
> + timespec_add_nsec(&r, &a, 1);
> + ZUC_ASSERT_EQ(1, r.tv_sec);
> + ZUC_ASSERT_EQ(0, r.tv_nsec);
> +
> + timespec_add_nsec(&r, &a, 2);
> + ZUC_ASSERT_EQ(1, r.tv_sec);
> + ZUC_ASSERT_EQ(1, r.tv_nsec);
> +
> + timespec_add_nsec(&r, &a, (NSEC_PER_SEC * 2ULL));
> + ZUC_ASSERT_EQ(2, r.tv_sec);
> + ZUC_ASSERT_EQ(NSEC_PER_SEC - 1, r.tv_nsec);
> +
> + timespec_add_nsec(&r, &a, (NSEC_PER_SEC * 2ULL) + 2);
> + ZUC_ASSERT_EQ(r.tv_sec, 3);
> + ZUC_ASSERT_EQ(r.tv_nsec, 1);
> +
All correct up to here.
> + a.tv_sec = 0;
> + a.tv_nsec = 1;
> + timespec_add_nsec(&r, &a, -2);
> + ZUC_ASSERT_EQ(r.tv_sec, 0);
> + ZUC_ASSERT_EQ(r.tv_nsec, -1);
Incorrect, tv_nsec cannot be negative.
> +
> + a.tv_nsec = 0;
> + timespec_add_nsec(&r, &a, -NSEC_PER_SEC);
> + ZUC_ASSERT_EQ(-1, r.tv_sec);
> + ZUC_ASSERT_EQ(0, r.tv_nsec);
Correct.
> +
> + a.tv_nsec = -1;
Invalid timestamp.
> + timespec_add_nsec(&r, &a, -NSEC_PER_SEC + 1);
> + ZUC_ASSERT_EQ(-1, r.tv_sec);
> + ZUC_ASSERT_EQ(0, r.tv_nsec);
> +
> + a.tv_nsec = 50;
> + timespec_add_nsec(&r, &a, (-NSEC_PER_SEC * 10ULL));
> + ZUC_ASSERT_EQ(-10, r.tv_sec);
> + ZUC_ASSERT_EQ(50, r.tv_nsec);
Correct.
> +
> + r.tv_sec = 4;
> + r.tv_nsec = 0;
> + timespec_add_nsec(&r, &r, NSEC_PER_SEC + 10ULL);
> + ZUC_ASSERT_EQ(5, r.tv_sec);
> + ZUC_ASSERT_EQ(10, r.tv_nsec);
> +
> + timespec_add_nsec(&r, &r, (NSEC_PER_SEC * 3ULL) - 9ULL);
> + ZUC_ASSERT_EQ(8, r.tv_sec);
> + ZUC_ASSERT_EQ(1, r.tv_nsec);
> +
> + timespec_add_nsec(&r, &r, (NSEC_PER_SEC * 7ULL) + (NSEC_PER_SEC -
> 1ULL));
> + ZUC_ASSERT_EQ(16, r.tv_sec);
> + ZUC_ASSERT_EQ(0, r.tv_nsec);
> +}
Correct.
Thanks,
pq
pgppZpE7OHQA5.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
