[libcamera-devel,v2,3/5] libcamera: file: Use Flags<> class for map flags
diff mbox series

Message ID 20210725171827.23643-4-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Add type-safe enum-based flags
Related show

Commit Message

Laurent Pinchart July 25, 2021, 5:18 p.m. UTC 
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@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@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(-)

Comments

Jacopo Mondi July 26, 2021, 9:52 a.m. UTC | #1 
Hi Laurent,

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@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@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 ?

>  	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 ?

> + */
> +
>  /**
>   * \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
>
Laurent Pinchart July 26, 2021, 10 a.m. UTC | #2 
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@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@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.

> >  	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 ?

> > + */
> > +
> >  /**
> >   * \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;
Jacopo Mondi July 26, 2021, 10:05 a.m. UTC | #3 
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@ideasonboard.com>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@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

Patch
diff mbox series 

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>;
+
 	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
+ */
+
 /**
  * \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;