| Message ID | 20250310170254.185172-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Mon, Mar 10, 2025 at 06:02:54PM +0100, Barnabás Pőcze wrote: > Only enums whose sizes match that of `int32_t` can be directly > supported. Otherwise the expected size and the real size would > disagree, leading to an assertion failure in `ControlValue::set()`. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks! > --- > include/libcamera/controls.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 7c7828ae5..4bfe9615c 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -125,7 +125,7 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> { > }; > > template<typename T> > -struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> { > +struct control_type<T, std::enable_if_t<std::is_enum_v<T> && sizeof(T) == sizeof(int32_t)>> : public control_type<int32_t> { However, for extra paranoia, the controls and properties enumerations are unscoped enum without an underlying specified type. I read that if an underlying type is not specified: "the underlying type is an implementation-defined integral type" https://en.cppreference.com/w/cpp/language/enum which makes me wonder if we should go extra paranoid and: --- a/include/libcamera/control_ids.h.in +++ b/include/libcamera/control_ids.h.in @@ -31,7 +31,7 @@ namespace {{vendor}} { {%- endif %} {% if ctrls %} -enum { +enum : int32_t { {%- for ctrl in ctrls %} {{ctrl.name|snake_case|upper}} = {{ctrl.id}}, {%- endfor %} > }; > > } /* namespace details */ > -- > 2.48.1 >
Quoting Barnabás Pőcze (2025-03-10 17:02:54) > Only enums whose sizes match that of `int32_t` can be directly > supported. Otherwise the expected size and the real size would > disagree, leading to an assertion failure in `ControlValue::set()`. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Has this happened? I assume if we try to make a control from an enum where we override the type it would now produce a compile failure (which I'm fine with for now). I assume we could make similar mappings of other enum sizes if we needed to at that point, but I don't see that as a requirement at the moment. This looks positively defensive so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> We're in a public header, but I don't think this is an API breaking change at all is it ? If this changed anything we'd see a compile failure already... > --- > include/libcamera/controls.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 7c7828ae5..4bfe9615c 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -125,7 +125,7 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> { > }; > > template<typename T> > -struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> { > +struct control_type<T, std::enable_if_t<std::is_enum_v<T> && sizeof(T) == sizeof(int32_t)>> : public control_type<int32_t> { > }; > > } /* namespace details */ > -- > 2.48.1 >
Quoting Kieran Bingham (2025-03-17 17:56:43) > Quoting Barnabás Pőcze (2025-03-10 17:02:54) > > Only enums whose sizes match that of `int32_t` can be directly > > supported. Otherwise the expected size and the real size would > > disagree, leading to an assertion failure in `ControlValue::set()`. > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > Has this happened? I assume if we try to make a control from an enum > where we override the type it would now produce a compile failure (which > I'm fine with for now). > > I assume we could make similar mappings of other enum sizes if we needed > to at that point, but I don't see that as a requirement at the moment. > > This looks positively defensive so: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > We're in a public header, but I don't think this is an API breaking > change at all is it ? If this changed anything we'd see a compile > failure already... > gitlab.freedesktop.org is starting to come back online \o/ but alas CI picked something up on this patch... https://gitlab.freedesktop.org/camera/libcamera/-/jobs/73022971 Could you check please? (it could be false positive still or something with the CI) -- Kieran > > > --- > > include/libcamera/controls.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index 7c7828ae5..4bfe9615c 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -125,7 +125,7 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> { > > }; > > > > template<typename T> > > -struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> { > > +struct control_type<T, std::enable_if_t<std::is_enum_v<T> && sizeof(T) == sizeof(int32_t)>> : public control_type<int32_t> { > > }; > > > > } /* namespace details */ > > -- > > 2.48.1 > >
Hi 2025. 03. 19. 11:48 keltezéssel, Kieran Bingham írta: > Quoting Kieran Bingham (2025-03-17 17:56:43) >> Quoting Barnabás Pőcze (2025-03-10 17:02:54) >>> Only enums whose sizes match that of `int32_t` can be directly >>> supported. Otherwise the expected size and the real size would >>> disagree, leading to an assertion failure in `ControlValue::set()`. >>> >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> >> Has this happened? I assume if we try to make a control from an enum >> where we override the type it would now produce a compile failure (which >> I'm fine with for now). >> >> I assume we could make similar mappings of other enum sizes if we needed >> to at that point, but I don't see that as a requirement at the moment. >> >> This looks positively defensive so: >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> We're in a public header, but I don't think this is an API breaking >> change at all is it ? If this changed anything we'd see a compile >> failure already... >> > > gitlab.freedesktop.org is starting to come back online \o/ > > but alas CI picked something up on this patch... > > https://gitlab.freedesktop.org/camera/libcamera/-/jobs/73022971 > > Could you check please? (it could be false positive still or something > with the CI) I am fairly sure that is unrelated. Regards, Barnabás Pőcze > > -- > Kieran > >> >>> --- >>> include/libcamera/controls.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >>> index 7c7828ae5..4bfe9615c 100644 >>> --- a/include/libcamera/controls.h >>> +++ b/include/libcamera/controls.h >>> @@ -125,7 +125,7 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> { >>> }; >>> >>> template<typename T> >>> -struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> { >>> +struct control_type<T, std::enable_if_t<std::is_enum_v<T> && sizeof(T) == sizeof(int32_t)>> : public control_type<int32_t> { >>> }; >>> >>> } /* namespace details */ >>> -- >>> 2.48.1 >>>
Quoting Barnabás Pőcze (2025-03-19 12:44:55) > Hi > > 2025. 03. 19. 11:48 keltezéssel, Kieran Bingham írta: > > Quoting Kieran Bingham (2025-03-17 17:56:43) > >> Quoting Barnabás Pőcze (2025-03-10 17:02:54) > >>> Only enums whose sizes match that of `int32_t` can be directly > >>> supported. Otherwise the expected size and the real size would > >>> disagree, leading to an assertion failure in `ControlValue::set()`. > >>> > >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> > >> Has this happened? I assume if we try to make a control from an enum > >> where we override the type it would now produce a compile failure (which > >> I'm fine with for now). > >> > >> I assume we could make similar mappings of other enum sizes if we needed > >> to at that point, but I don't see that as a requirement at the moment. > >> > >> This looks positively defensive so: > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >> We're in a public header, but I don't think this is an API breaking > >> change at all is it ? If this changed anything we'd see a compile > >> failure already... > >> > > > > gitlab.freedesktop.org is starting to come back online \o/ > > > > but alas CI picked something up on this patch... > > > > https://gitlab.freedesktop.org/camera/libcamera/-/jobs/73022971 > > > > Could you check please? (it could be false positive still or something > > with the CI) > > I am fairly sure that is unrelated. > Agreed, I re-ran the tests and they've passed, So I'm fine with this patch being merged. -- Kieran > > Regards, > Barnabás Pőcze > > > > > > -- > > Kieran > > > >> > >>> --- > >>> include/libcamera/controls.h | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > >>> index 7c7828ae5..4bfe9615c 100644 > >>> --- a/include/libcamera/controls.h > >>> +++ b/include/libcamera/controls.h > >>> @@ -125,7 +125,7 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> { > >>> }; > >>> > >>> template<typename T> > >>> -struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> { > >>> +struct control_type<T, std::enable_if_t<std::is_enum_v<T> && sizeof(T) == sizeof(int32_t)>> : public control_type<int32_t> { > >>> }; > >>> > >>> } /* namespace details */ > >>> -- > >>> 2.48.1 > >>> >
Hi 2025. 03. 17. 18:56 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Mon, Mar 10, 2025 at 06:02:54PM +0100, Barnabás Pőcze wrote: >> Only enums whose sizes match that of `int32_t` can be directly >> supported. Otherwise the expected size and the real size would >> disagree, leading to an assertion failure in `ControlValue::set()`. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks! > >> --- >> include/libcamera/controls.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >> index 7c7828ae5..4bfe9615c 100644 >> --- a/include/libcamera/controls.h >> +++ b/include/libcamera/controls.h >> @@ -125,7 +125,7 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> { >> }; >> >> template<typename T> >> -struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> { >> +struct control_type<T, std::enable_if_t<std::is_enum_v<T> && sizeof(T) == sizeof(int32_t)>> : public control_type<int32_t> { > > However, for extra paranoia, the controls and properties enumerations > are unscoped enum without an underlying specified type. > > I read that if an underlying type is not specified: > "the underlying type is an implementation-defined integral type" > https://en.cppreference.com/w/cpp/language/enum > > which makes me wonder if we should go extra paranoid and: > > --- a/include/libcamera/control_ids.h.in > +++ b/include/libcamera/control_ids.h.in > @@ -31,7 +31,7 @@ namespace {{vendor}} { > {%- endif %} > > {% if ctrls %} > -enum { > +enum : int32_t { > {%- for ctrl in ctrls %} > {{ctrl.name|snake_case|upper}} = {{ctrl.id}}, > {%- endfor %} > >> }; >> >> } /* namespace details */ I think that could be a good idea, maybe something like this: diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in index 8f037589d..d56cb08ee 100644 --- a/include/libcamera/control_ids.h.in +++ b/include/libcamera/control_ids.h.in @@ -31,7 +31,7 @@ namespace {{vendor}} { {%- endif %} {% if ctrls %} -enum { +enum : unsigned int { {%- for ctrl in ctrls %} {{ctrl.name|snake_case|upper}} = {{ctrl.id}}, {%- endfor %} @@ -40,7 +40,7 @@ enum { {% for ctrl in ctrls -%} {% if ctrl.is_enum -%} -enum {{ctrl.name}}Enum { +enum {{ctrl.name}}Enum : int32_t { {%- for enum in ctrl.enum_values %} {{enum.name}} = {{enum.value}}, {%- endfor %} that is, control identifiers have the underlying type `unsigned int` since that is what `ControlList`, etc. use for this purpose, and the specific enumerators have type `int32_t` since that is what `ControlValue`, etc. use. Regards, Barnabás Pőcze >> -- >> 2.48.1 >>
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 7c7828ae5..4bfe9615c 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -125,7 +125,7 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> { }; template<typename T> -struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> { +struct control_type<T, std::enable_if_t<std::is_enum_v<T> && sizeof(T) == sizeof(int32_t)>> : public control_type<int32_t> { }; } /* namespace details */
Only enums whose sizes match that of `int32_t` can be directly supported. Otherwise the expected size and the real size would disagree, leading to an assertion failure in `ControlValue::set()`. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/controls.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)