[{"id":18348,"web_url":"https://patchwork.libcamera.org/comment/18348/","msgid":"<20210726095224.p7wnonjtmunu35rd@uno.localdomain>","date":"2021-07-26T09:52:24","subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: file: Use Flags<>\n\tclass for map flags","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Jul 25, 2021 at 08:18:25PM +0300, Laurent Pinchart wrote:\n> Use the newly introduced Flags<> class to store a bitfield of\n> File::MapFlag in a type-safe way.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Niklas Söderlund \n> ---\n> Changes since v1:\n>\n> - Use an unscoped enum\n> ---\n> include/libcamera/base/file.h | 5 ++++-\n> src/libcamera/base/file.cpp | 7 ++++++-\n> 2 files changed, 10 insertions(+), 2 deletions(-)\n>\n> diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> index 7dd1559d2285..452a658b409f 100644\n> --- a/include/libcamera/base/file.h\n> +++ b/include/libcamera/base/file.h\n> @@ -15,6 +15,7 @@\n> #include \n>\n> #include \n> +#include \n> #include \n>\n> namespace libcamera {\n> @@ -27,6 +28,8 @@ public:\n> \t\tMapPrivate = (1 << 0),\n> \t};\n>\n> +\tusing MapFlags = Flags;\n> +\n\nShouldn't you enable bitwise operations for this type ?\n\n> \tenum OpenMode {\n> \t\tNotOpen = 0,\n> \t\tReadOnly = (1 << 0),\n> @@ -57,7 +60,7 @@ public:\n> \tssize_t write(const Span &data);\n>\n> \tSpan map(off_t offset = 0, ssize_t size = -1,\n> -\t\t\t MapFlag flags = MapNoOption);\n> +\t\t\t MapFlags flags = MapNoOption);\n> \tbool unmap(uint8_t *addr);\n>\n> \tstatic bool exists(const std::string &name);\n> diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp\n> index 073666fa6f66..e180c5479098 100644\n> --- a/src/libcamera/base/file.cpp\n> +++ b/src/libcamera/base/file.cpp\n> @@ -52,6 +52,11 @@ LOG_DEFINE_CATEGORY(File)\n> * the file constents\n> */\n>\n> +/**\n> + * \\typedef File::MapFlags\n> + * \\brief An OR combination of File::MapFlag values\n\nWhy a type is an OR combination ?\n\n> + */\n> +\n> /**\n> * \\enum File::OpenMode\n> * \\brief Mode in which a file is opened\n> @@ -371,7 +376,7 @@ ssize_t File::write(const Span &data)\n> *\n> * \\return The mapped memory on success, or an empty span otherwise\n> */\n> -Span File::map(off_t offset, ssize_t size, enum File::MapFlag flags)\n> +Span File::map(off_t offset, ssize_t size, File::MapFlags flags)\n> {\n> \tif (!isOpen()) {\n> \t\terror_ = -EBADF;\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 10C05C322C\n\tfor ;\n\tMon, 26 Jul 2021 09:51:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C9D42687BA;\n\tMon, 26 Jul 2021 11:51:38 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C005B60272\n\tfor ;\n\tMon, 26 Jul 2021 11:51:37 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 47B8160007;\n\tMon, 26 Jul 2021 09:51:36 +0000 (UTC)"],"Date":"Mon, 26 Jul 2021 11:52:24 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210726095224.p7wnonjtmunu35rd@uno.localdomain>","References":"<20210725171827.23643-1-laurent.pinchart@ideasonboard.com>\n\t<20210725171827.23643-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210725171827.23643-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: file: Use Flags<>\n\tclass for map flags","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18351,"web_url":"https://patchwork.libcamera.org/comment/18351/","msgid":"","date":"2021-07-26T10:00:11","subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: file: Use Flags<>\n\tclass for map flags","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jul 26, 2021 at 11:52:24AM +0200, Jacopo Mondi wrote:\n> On Sun, Jul 25, 2021 at 08:18:25PM +0300, Laurent Pinchart wrote:\n> > Use the newly introduced Flags<> class to store a bitfield of\n> > File::MapFlag in a type-safe way.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Niklas Söderlund \n> > ---\n> > Changes since v1:\n> >\n> > - Use an unscoped enum\n> > ---\n> > include/libcamera/base/file.h | 5 ++++-\n> > src/libcamera/base/file.cpp | 7 ++++++-\n> > 2 files changed, 10 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> > index 7dd1559d2285..452a658b409f 100644\n> > --- a/include/libcamera/base/file.h\n> > +++ b/include/libcamera/base/file.h\n> > @@ -15,6 +15,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> >\n> > namespace libcamera {\n> > @@ -27,6 +28,8 @@ public:\n> > \t\tMapPrivate = (1 << 0),\n> > \t};\n> >\n> > +\tusing MapFlags = Flags;\n> > +\n> \n> Shouldn't you enable bitwise operations for this type ?\n\nNot for Flags<>, that's handled by the Flags class itself. It's only\noperators for the base Flag that need to be enabled, and only when the\nbase Flag is a scoped enum. As File uses unscoped enums, it's not needed\nhere. This is changed in 5/5.\n\n> > \tenum OpenMode {\n> > \t\tNotOpen = 0,\n> > \t\tReadOnly = (1 << 0),\n> > @@ -57,7 +60,7 @@ public:\n> > \tssize_t write(const Span &data);\n> >\n> > \tSpan map(off_t offset = 0, ssize_t size = -1,\n> > -\t\t\t MapFlag flags = MapNoOption);\n> > +\t\t\t MapFlags flags = MapNoOption);\n> > \tbool unmap(uint8_t *addr);\n> >\n> > \tstatic bool exists(const std::string &name);\n> > diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp\n> > index 073666fa6f66..e180c5479098 100644\n> > --- a/src/libcamera/base/file.cpp\n> > +++ b/src/libcamera/base/file.cpp\n> > @@ -52,6 +52,11 @@ LOG_DEFINE_CATEGORY(File)\n> > * the file constents\n> > */\n> >\n> > +/**\n> > + * \\typedef File::MapFlags\n> > + * \\brief An OR combination of File::MapFlag values\n> \n> Why a type is an OR combination ?\n\nMaybe \"A bitwise combination of File::MapFlag values\" would be better ?\n\n> > + */\n> > +\n> > /**\n> > * \\enum File::OpenMode\n> > * \\brief Mode in which a file is opened\n> > @@ -371,7 +376,7 @@ ssize_t File::write(const Span &data)\n> > *\n> > * \\return The mapped memory on success, or an empty span otherwise\n> > */\n> > -Span File::map(off_t offset, ssize_t size, enum File::MapFlag flags)\n> > +Span File::map(off_t offset, ssize_t size, File::MapFlags flags)\n> > {\n> > \tif (!isOpen()) {\n> > \t\terror_ = -EBADF;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 43D4CC0109\n\tfor ;\n\tMon, 26 Jul 2021 10:00:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EAD7468548;\n\tMon, 26 Jul 2021 12:00:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 041CC68536\n\tfor ;\n\tMon, 26 Jul 2021 12:00:18 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 21FFC332;\n\tMon, 26 Jul 2021 12:00:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EDc2ok5H\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627293617;\n\tbh=xSw9Hmcztz5bf37gvEadzmixp46xsK9qjgc1s/RAxZ0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EDc2ok5Hh+LMWOYdaTX95R4E6o/2xflVdtld4O7Tt/FiYs8i5klheBhd/izaxLKIl\n\tgu7rfdUNGqsAQ3BK+TZFlmn+tpnftpGJc4y9cjrrN5xOfqNwIrpIRpZrkH6EVm3+e4\n\tmjMCR1HZVxw3v4lPWPphWx8pl4GGgCJEFcbFRUYA=","Date":"Mon, 26 Jul 2021 13:00:11 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210725171827.23643-1-laurent.pinchart@ideasonboard.com>\n\t<20210725171827.23643-4-laurent.pinchart@ideasonboard.com>\n\t<20210726095224.p7wnonjtmunu35rd@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210726095224.p7wnonjtmunu35rd@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: file: Use Flags<>\n\tclass for map flags","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18352,"web_url":"https://patchwork.libcamera.org/comment/18352/","msgid":"<20210726100520.ix5mww45fmxltds7@uno.localdomain>","date":"2021-07-26T10:05:20","subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: file: Use Flags<>\n\tclass for map flags","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Mon, Jul 26, 2021 at 01:00:11PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Jul 26, 2021 at 11:52:24AM +0200, Jacopo Mondi wrote:\n> > On Sun, Jul 25, 2021 at 08:18:25PM +0300, Laurent Pinchart wrote:\n> > > Use the newly introduced Flags<> class to store a bitfield of\n> > > File::MapFlag in a type-safe way.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > Reviewed-by: Niklas Söderlund \n> > > ---\n> > > Changes since v1:\n> > >\n> > > - Use an unscoped enum\n> > > ---\n> > > include/libcamera/base/file.h | 5 ++++-\n> > > src/libcamera/base/file.cpp | 7 ++++++-\n> > > 2 files changed, 10 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> > > index 7dd1559d2285..452a658b409f 100644\n> > > --- a/include/libcamera/base/file.h\n> > > +++ b/include/libcamera/base/file.h\n> > > @@ -15,6 +15,7 @@\n> > > #include \n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > namespace libcamera {\n> > > @@ -27,6 +28,8 @@ public:\n> > > \t\tMapPrivate = (1 << 0),\n> > > \t};\n> > >\n> > > +\tusing MapFlags = Flags;\n> > > +\n> >\n> > Shouldn't you enable bitwise operations for this type ?\n>\n> Not for Flags<>, that's handled by the Flags class itself. It's only\n> operators for the base Flag that need to be enabled, and only when the\n> base Flag is a scoped enum. As File uses unscoped enums, it's not needed\n> here. This is changed in 5/5.\n\nI got there after writing this :)\n\n>\n> > > \tenum OpenMode {\n> > > \t\tNotOpen = 0,\n> > > \t\tReadOnly = (1 << 0),\n> > > @@ -57,7 +60,7 @@ public:\n> > > \tssize_t write(const Span &data);\n> > >\n> > > \tSpan map(off_t offset = 0, ssize_t size = -1,\n> > > -\t\t\t MapFlag flags = MapNoOption);\n> > > +\t\t\t MapFlags flags = MapNoOption);\n> > > \tbool unmap(uint8_t *addr);\n> > >\n> > > \tstatic bool exists(const std::string &name);\n> > > diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp\n> > > index 073666fa6f66..e180c5479098 100644\n> > > --- a/src/libcamera/base/file.cpp\n> > > +++ b/src/libcamera/base/file.cpp\n> > > @@ -52,6 +52,11 @@ LOG_DEFINE_CATEGORY(File)\n> > > * the file constents\n> > > */\n> > >\n> > > +/**\n> > > + * \\typedef File::MapFlags\n> > > + * \\brief An OR combination of File::MapFlag values\n> >\n> > Why a type is an OR combination ?\n>\n> Maybe \"A bitwise combination of File::MapFlag values\" would be better ?\n>\n\nYeah, sorry I got confused by the OR operation being mentioned.\nWhatever is fine\n\n> > > + */\n> > > +\n> > > /**\n> > > * \\enum File::OpenMode\n> > > * \\brief Mode in which a file is opened\n> > > @@ -371,7 +376,7 @@ ssize_t File::write(const Span &data)\n> > > *\n> > > * \\return The mapped memory on success, or an empty span otherwise\n> > > */\n> > > -Span File::map(off_t offset, ssize_t size, enum File::MapFlag flags)\n> > > +Span File::map(off_t offset, ssize_t size, File::MapFlags flags)\n> > > {\n> > > \tif (!isOpen()) {\n> > > \t\terror_ = -EBADF;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 30BFAC0109\n\tfor ;\n\tMon, 26 Jul 2021 10:04:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 91D4C687B3;\n\tMon, 26 Jul 2021 12:04:33 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E8EF768536\n\tfor ;\n\tMon, 26 Jul 2021 12:04:32 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 6E6A460002;\n\tMon, 26 Jul 2021 10:04:32 +0000 (UTC)"],"Date":"Mon, 26 Jul 2021 12:05:20 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210726100520.ix5mww45fmxltds7@uno.localdomain>","References":"<20210725171827.23643-1-laurent.pinchart@ideasonboard.com>\n\t<20210725171827.23643-4-laurent.pinchart@ideasonboard.com>\n\t<20210726095224.p7wnonjtmunu35rd@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: file: Use Flags<>\n\tclass for map flags","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]