[libcamera-devel] [PATCH v2 3/5] libcamera: file: Use Flags<> class for map flags
Jacopo Mondi
jacopo at jmondi.org
Mon Jul 26 12:05:20 CEST 2021
On Mon, Jul 26, 2021 at 01:00:11PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Jul 26, 2021 at 11:52:24AM +0200, Jacopo Mondi wrote:
> > On Sun, Jul 25, 2021 at 08:18:25PM +0300, Laurent Pinchart wrote:
> > > Use the newly introduced Flags<> class to store a bitfield of
> > > File::MapFlag in a type-safe way.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > > Changes since v1:
> > >
> > > - Use an unscoped enum
> > > ---
> > > include/libcamera/base/file.h | 5 ++++-
> > > src/libcamera/base/file.cpp | 7 ++++++-
> > > 2 files changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h
> > > index 7dd1559d2285..452a658b409f 100644
> > > --- a/include/libcamera/base/file.h
> > > +++ b/include/libcamera/base/file.h
> > > @@ -15,6 +15,7 @@
> > > #include <libcamera/base/private.h>
> > >
> > > #include <libcamera/base/class.h>
> > > +#include <libcamera/base/flags.h>
> > > #include <libcamera/base/span.h>
> > >
> > > namespace libcamera {
> > > @@ -27,6 +28,8 @@ public:
> > > MapPrivate = (1 << 0),
> > > };
> > >
> > > + using MapFlags = Flags<MapFlag>;
> > > +
> >
> > Shouldn't you enable bitwise operations for this type ?
>
> Not for Flags<>, that's handled by the Flags class itself. It's only
> operators for the base Flag that need to be enabled, and only when the
> base Flag is a scoped enum. As File uses unscoped enums, it's not needed
> here. This is changed in 5/5.
I got there after writing this :)
>
> > > enum OpenMode {
> > > NotOpen = 0,
> > > ReadOnly = (1 << 0),
> > > @@ -57,7 +60,7 @@ public:
> > > ssize_t write(const Span<const uint8_t> &data);
> > >
> > > Span<uint8_t> map(off_t offset = 0, ssize_t size = -1,
> > > - MapFlag flags = MapNoOption);
> > > + MapFlags flags = MapNoOption);
> > > bool unmap(uint8_t *addr);
> > >
> > > static bool exists(const std::string &name);
> > > diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp
> > > index 073666fa6f66..e180c5479098 100644
> > > --- a/src/libcamera/base/file.cpp
> > > +++ b/src/libcamera/base/file.cpp
> > > @@ -52,6 +52,11 @@ LOG_DEFINE_CATEGORY(File)
> > > * the file constents
> > > */
> > >
> > > +/**
> > > + * \typedef File::MapFlags
> > > + * \brief An OR combination of File::MapFlag values
> >
> > Why a type is an OR combination ?
>
> Maybe "A bitwise combination of File::MapFlag values" would be better ?
>
Yeah, sorry I got confused by the OR operation being mentioned.
Whatever is fine
> > > + */
> > > +
> > > /**
> > > * \enum File::OpenMode
> > > * \brief Mode in which a file is opened
> > > @@ -371,7 +376,7 @@ ssize_t File::write(const Span<const uint8_t> &data)
> > > *
> > > * \return The mapped memory on success, or an empty span otherwise
> > > */
> > > -Span<uint8_t> File::map(off_t offset, ssize_t size, enum File::MapFlag flags)
> > > +Span<uint8_t> File::map(off_t offset, ssize_t size, File::MapFlags flags)
> > > {
> > > if (!isOpen()) {
> > > error_ = -EBADF;
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list