| Message ID | 20260903091843.85548-3-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Thu, Sep 03, 2026 at 11:18:40AM +0200, Barnabás Pőcze wrote: > Bit fields are inconvenient to use with references, e.g. `std::swap()`, > so remove them from the `Control` type as a preparation. > > `size_t : 32` is replaced with just `uint32_t`, and `ControlType`'s > underlying type is forced to be `uint8_t` so that bit field can also be > removed. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Thank you for addressing my comment. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/controls.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index da73e41848..e4d5d13a2c 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -26,7 +26,7 @@ namespace libcamera { > > class ControlValidator; > > -enum ControlType { > +enum ControlType : uint8_t { > ControlTypeNone, > ControlTypeBool, > ControlTypeByte, > @@ -242,9 +242,9 @@ public: > std::size_t numElements = 1); > > private: > - ControlType type_ : 8; > + ControlType type_; > bool isArray_; > - std::size_t numElements_ : 32; > + uint32_t numElements_; > union { > uint64_t internal; > void *external;
Quoting Barnabás Pőcze (2026-09-03 10:18:40) > Bit fields are inconvenient to use with references, e.g. `std::swap()`, > so remove them from the `Control` type as a preparation. > > `size_t : 32` is replaced with just `uint32_t`, and `ControlType`'s > underlying type is forced to be `uint8_t` so that bit field can also be > removed. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> I'd still like to try to capture API/ABI changes in the commit messages. It would help a lot for when we get to release notes, and if they're known at submission time - it helps to highlight ABI breakage for review I think. ABI: sizeof(ControlType) structure may change. ABI: Bitfields conversion may impact some compilers. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/controls.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index da73e41848..e4d5d13a2c 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -26,7 +26,7 @@ namespace libcamera { > > class ControlValidator; > > -enum ControlType { > +enum ControlType : uint8_t { > ControlTypeNone, > ControlTypeBool, > ControlTypeByte, > @@ -242,9 +242,9 @@ public: > std::size_t numElements = 1); > > private: > - ControlType type_ : 8; > + ControlType type_; > bool isArray_; > - std::size_t numElements_ : 32; > + uint32_t numElements_; > union { > uint64_t internal; > void *external; > -- > 2.55.0 >
On Thu, Sep 03, 2026 at 12:54:01PM +0100, Kieran Bingham wrote: > Quoting Barnabás Pőcze (2026-09-03 10:18:40) > > Bit fields are inconvenient to use with references, e.g. `std::swap()`, > > so remove them from the `Control` type as a preparation. > > > > `size_t : 32` is replaced with just `uint32_t`, and `ControlType`'s > > underlying type is forced to be `uint8_t` so that bit field can also be > > removed. > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > I'd still like to try to capture API/ABI changes in the commit messages. > It would help a lot for when we get to release notes, and if they're > known at submission time - it helps to highlight ABI breakage for review > I think. > > ABI: sizeof(ControlType) structure may change. > ABI: Bitfields conversion may impact some compilers. No objection, but let's then update the documentation. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > include/libcamera/controls.h | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index da73e41848..e4d5d13a2c 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -26,7 +26,7 @@ namespace libcamera { > > > > class ControlValidator; > > > > -enum ControlType { > > +enum ControlType : uint8_t { > > ControlTypeNone, > > ControlTypeBool, > > ControlTypeByte, > > @@ -242,9 +242,9 @@ public: > > std::size_t numElements = 1); > > > > private: > > - ControlType type_ : 8; > > + ControlType type_; > > bool isArray_; > > - std::size_t numElements_ : 32; > > + uint32_t numElements_; > > union { > > uint64_t internal; > > void *external;
2026. 09. 03. 14:13 keltezéssel, Laurent Pinchart írta: > On Thu, Sep 03, 2026 at 12:54:01PM +0100, Kieran Bingham wrote: >> Quoting Barnabás Pőcze (2026-09-03 10:18:40) >>> Bit fields are inconvenient to use with references, e.g. `std::swap()`, >>> so remove them from the `Control` type as a preparation. >>> >>> `size_t : 32` is replaced with just `uint32_t`, and `ControlType`'s >>> underlying type is forced to be `uint8_t` so that bit field can also be >>> removed. >>> >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> >> I'd still like to try to capture API/ABI changes in the commit messages. >> It would help a lot for when we get to release notes, and if they're >> known at submission time - it helps to highlight ABI breakage for review >> I think. >> >> ABI: sizeof(ControlType) structure may change. >> ABI: Bitfields conversion may impact some compilers. > > No objection, but let's then update the documentation. `checkstyle.py` will also report Invalid commit trailer key '...' so what should be done? > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> --- >>> include/libcamera/controls.h | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >>> index da73e41848..e4d5d13a2c 100644 >>> --- a/include/libcamera/controls.h >>> +++ b/include/libcamera/controls.h >>> @@ -26,7 +26,7 @@ namespace libcamera { >>> >>> class ControlValidator; >>> >>> -enum ControlType { >>> +enum ControlType : uint8_t { >>> ControlTypeNone, >>> ControlTypeBool, >>> ControlTypeByte, >>> @@ -242,9 +242,9 @@ public: >>> std::size_t numElements = 1); >>> >>> private: >>> - ControlType type_ : 8; >>> + ControlType type_; >>> bool isArray_; >>> - std::size_t numElements_ : 32; >>> + uint32_t numElements_; >>> union { >>> uint64_t internal; >>> void *external; >
On Thu, Sep 03, 2026 at 03:13:36PM +0200, Barnabás Pőcze wrote: > 2026. 09. 03. 14:13 keltezéssel, Laurent Pinchart írta: > > On Thu, Sep 03, 2026 at 12:54:01PM +0100, Kieran Bingham wrote: > >> Quoting Barnabás Pőcze (2026-09-03 10:18:40) > >>> Bit fields are inconvenient to use with references, e.g. `std::swap()`, > >>> so remove them from the `Control` type as a preparation. > >>> > >>> `size_t : 32` is replaced with just `uint32_t`, and `ControlType`'s > >>> underlying type is forced to be `uint8_t` so that bit field can also be > >>> removed. > >>> > >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> > >> I'd still like to try to capture API/ABI changes in the commit messages. > >> It would help a lot for when we get to release notes, and if they're > >> known at submission time - it helps to highlight ABI breakage for review > >> I think. > >> > >> ABI: sizeof(ControlType) structure may change. > >> ABI: Bitfields conversion may impact some compilers. > > > > No objection, but let's then update the documentation. > > `checkstyle.py` will also report > > Invalid commit trailer key '...' > > so what should be done? We should then update checkstyle.py, if that's the desired format :-) I wouldn't block this patch series for that though, we can start using the ABI tag once the documentation and script have been updated. > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >>> --- > >>> include/libcamera/controls.h | 6 +++--- > >>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > >>> index da73e41848..e4d5d13a2c 100644 > >>> --- a/include/libcamera/controls.h > >>> +++ b/include/libcamera/controls.h > >>> @@ -26,7 +26,7 @@ namespace libcamera { > >>> > >>> class ControlValidator; > >>> > >>> -enum ControlType { > >>> +enum ControlType : uint8_t { > >>> ControlTypeNone, > >>> ControlTypeBool, > >>> ControlTypeByte, > >>> @@ -242,9 +242,9 @@ public: > >>> std::size_t numElements = 1); > >>> > >>> private: > >>> - ControlType type_ : 8; > >>> + ControlType type_; > >>> bool isArray_; > >>> - std::size_t numElements_ : 32; > >>> + uint32_t numElements_; > >>> union { > >>> uint64_t internal; > >>> void *external;
Quoting Laurent Pinchart (2026-09-03 14:23:15) > On Thu, Sep 03, 2026 at 03:13:36PM +0200, Barnabás Pőcze wrote: > > 2026. 09. 03. 14:13 keltezéssel, Laurent Pinchart írta: > > > On Thu, Sep 03, 2026 at 12:54:01PM +0100, Kieran Bingham wrote: > > >> Quoting Barnabás Pőcze (2026-09-03 10:18:40) > > >>> Bit fields are inconvenient to use with references, e.g. `std::swap()`, > > >>> so remove them from the `Control` type as a preparation. > > >>> > > >>> `size_t : 32` is replaced with just `uint32_t`, and `ControlType`'s > > >>> underlying type is forced to be `uint8_t` so that bit field can also be > > >>> removed. > > >>> > > >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > >> > > >> I'd still like to try to capture API/ABI changes in the commit messages. > > >> It would help a lot for when we get to release notes, and if they're > > >> known at submission time - it helps to highlight ABI breakage for review > > >> I think. > > >> > > >> ABI: sizeof(ControlType) structure may change. > > >> ABI: Bitfields conversion may impact some compilers. > > > > > > No objection, but let's then update the documentation. > > > > `checkstyle.py` will also report > > > > Invalid commit trailer key '...' > > > > so what should be done? > > We should then update checkstyle.py, if that's the desired format :-) I > wouldn't block this patch series for that though, we can start using the > ABI tag once the documentation and script have been updated. No time like the present. I put a proposal on the list. But it doesn't have to block this series, you don't have to add the tags. I just think it's something useful for tracking, and means we'll have a way to get the CI to block ABI breakages (and then allow them when they are marked as expected) -- Kieran > > > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> > > >>> --- > > >>> include/libcamera/controls.h | 6 +++--- > > >>> 1 file changed, 3 insertions(+), 3 deletions(-) > > >>> > > >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > >>> index da73e41848..e4d5d13a2c 100644 > > >>> --- a/include/libcamera/controls.h > > >>> +++ b/include/libcamera/controls.h > > >>> @@ -26,7 +26,7 @@ namespace libcamera { > > >>> > > >>> class ControlValidator; > > >>> > > >>> -enum ControlType { > > >>> +enum ControlType : uint8_t { > > >>> ControlTypeNone, > > >>> ControlTypeBool, > > >>> ControlTypeByte, > > >>> @@ -242,9 +242,9 @@ public: > > >>> std::size_t numElements = 1); > > >>> > > >>> private: > > >>> - ControlType type_ : 8; > > >>> + ControlType type_; > > >>> bool isArray_; > > >>> - std::size_t numElements_ : 32; > > >>> + uint32_t numElements_; > > >>> union { > > >>> uint64_t internal; > > >>> void *external; > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index da73e41848..e4d5d13a2c 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -26,7 +26,7 @@ namespace libcamera { class ControlValidator; -enum ControlType { +enum ControlType : uint8_t { ControlTypeNone, ControlTypeBool, ControlTypeByte, @@ -242,9 +242,9 @@ public: std::size_t numElements = 1); private: - ControlType type_ : 8; + ControlType type_; bool isArray_; - std::size_t numElements_ : 32; + uint32_t numElements_; union { uint64_t internal; void *external;
Bit fields are inconvenient to use with references, e.g. `std::swap()`, so remove them from the `Control` type as a preparation. `size_t : 32` is replaced with just `uint32_t`, and `ControlType`'s underlying type is forced to be `uint8_t` so that bit field can also be removed. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/controls.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)