[libcamera-devel] [PATCH] libcamera: utils: Add arithmetic operators for struct timespec

[Shik Chen]

Hi Laurent,

On Thu, Jan 24, 2019 at 12:04 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Shik,
>
> On Wed, Jan 23, 2019 at 11:00:21PM +0800, Shik Chen wrote:
> > On Wed, Jan 23, 2019 at 4:28 PM Laurent Pinchart <
> > laurent.pinchart at ideasonboard.com> wrote:
> > >
> > > Implement additions and subtraction of struct timespec through
> > > non-member operators to simplify timespec handling.
> >
> > IIUC, operator overloading on types of other libs is banned by the style guide
> > :Q
> > https://google.github.io/styleguide/cppguide.html#Operator_Overloading
>
> But it also states "Don't go out of your way to avoid defining operator
> overloads. For example, prefer to define ==, =, and <<, rather than
> Equals(), CopyFrom(), and PrintTo().". Would you recommend create
> timespecAdd() and timespecSub() functions instead ? I think this is a
> good case for operator overloading, as adding or subtracting timespec
> structures will be rejected if utils.h isn't included (so no risk of
> different behaviours unknown to the developer), and the + and -
> operators map very well to what we need to do.

IMO the rule "Define operators only on your own types" has higher precedence.
Two libraries might have conflict definitions if they violate this rule.

>
> For reference, my use case was
>
>         struct timespec start;
>         clock_gettime(CLOCK_MONOTINIC, &start);
>
>         ...
>
>         struct timespec now;
>         clock_gettime(CLOCK_MONOTINIC, &now);
>         struct timespec timeout = old_timeout + start - now;
>
> which could be written
>
>         struct timespec timeout = old_timeout;
>         timespecAdd(&timeout, start);
>         timespecSub(&timeout, now);
>
> but is a bit less convenient.
>
> What do you advise ?

Ideally most unavoidable C fashion code snippets should be wrapped in idiomatic
C++ if possible. Can we replace it with standard C++ types such as
std::chrono::duration and std::chrono::time_point, which already provide
arithmetic operators? Using things in std::chrono is a little bit verbose and
template heavy though (it's too flexible...).

Another option is to create/borrow some time manipulating classes and only
convert to C structs when needed. A good example is absl time library [1] [2],
which has a nicer API than std::chrono and can be easily converted to/from the
types in std::chrono/timespec/timeval.

[1] https://abseil.io/docs/cpp/guides/time
[2] https://github.com/abseil/abseil-cpp/blob/master/absl/time/time.h

>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >
> > > I wrote this code while working on EINTR handling for the event
> > > dispatcher, and later refactored the implementation in a way that
> > > doesn't make use of these operators anymore. I thought they could still
> > > be useful as reference for later use. This patch is thus not meant to be
> > > merged now, but can be picked up later if anyone needs it.
> > >
> > >  src/libcamera/include/utils.h |  6 +++++
> > >  src/libcamera/meson.build     |  1 +
> > >  src/libcamera/utils.cpp       | 46 +++++++++++++++++++++++++++++++++++
> > >  3 files changed, 53 insertions(+)
> > >  create mode 100644 src/libcamera/utils.cpp
> > >
> > > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> > > index 73fa2e69b029..7df20976f36f 100644
> > > --- a/src/libcamera/include/utils.h
> > > +++ b/src/libcamera/include/utils.h
> > > @@ -8,6 +8,7 @@
> > >  #define __LIBCAMERA_UTILS_H__
> > >
> > >  #include <memory>
> > > +#include <time.h>
> > >
> > >  #define ARRAY_SIZE(a)  (sizeof(a) / sizeof(a[0]))
> > >
> > > @@ -24,6 +25,11 @@ std::unique_ptr<T> make_unique(Args&&... args)
> > >
> > >  } /* namespace utils */
> > >
> > > +struct timespec &operator+=(struct timespec &lhs, const struct timespec &
> > rhs);
> > > +struct timespec operator+(struct timespec lhs, const struct timespec &rhs);
> > > +struct timespec &operator-=(struct timespec &lhs, const struct timespec &
> > rhs);
> > > +struct timespec operator-(struct timespec lhs, const struct timespec &rhs);
> > > +
> > >  } /* namespace libcamera */
> > >
> > >  #endif /* __LIBCAMERA_UTILS_H__ */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index f9f25c0ecf15..72d4a194e19a 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -11,6 +11,7 @@ libcamera_sources = files([
> > >      'pipeline_handler.cpp',
> > >      'signal.cpp',
> > >      'timer.cpp',
> > > +    'utils.cpp',
> > >      'v4l2_device.cpp',
> > >  ])
> > >
> > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> > > new file mode 100644
> > > index 000000000000..5014b27d8c7d
> > > --- /dev/null
> > > +++ b/src/libcamera/utils.cpp
> > > @@ -0,0 +1,46 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2018, Google Inc.
> > > + *
> > > + * utils.cpp - Miscellaneous utility functions
> > > + */
> > > +
> > > +#include "utils.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +struct timespec &operator+=(struct timespec &lhs, const struct timespec &
> > rhs)
> > > +{
> > > +       lhs.tv_sec += rhs.tv_sec;
> > > +       lhs.tv_nsec += rhs.tv_nsec;
> > > +       if (lhs.tv_nsec >= 1000000000) {
> > > +               lhs.tv_sec++;
> > > +               lhs.tv_nsec -= 1000000000;
> > > +       }
> > > +
> > > +       return lhs;
> > > +}
> > > +
> > > +struct timespec operator+(struct timespec lhs, const struct timespec &rhs)
> > > +{
> > > +       return lhs += rhs;
> > > +}
> > > +
> > > +struct timespec &operator-=(struct timespec &lhs, const struct timespec &
> > rhs)
> > > +{
> > > +       lhs.tv_sec -= rhs.tv_sec;
> > > +       lhs.tv_nsec -= rhs.tv_nsec;
> > > +       if (lhs.tv_nsec < 0) {
> > > +               lhs.tv_sec--;
> > > +               lhs.tv_nsec += 1000000000;
> > > +       }
> > > +
> > > +       return lhs;
> > > +}
> > > +
> > > +struct timespec operator-(struct timespec lhs, const struct timespec &rhs)
> > > +{
> > > +       return lhs - rhs;
> > > +}
> > > +
> > > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart

Sincerely,
Shik