| Message ID | 20200301160254.4548-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 01/03/2020 16:02, Laurent Pinchart wrote: > Replace the default case with an explicit ControlTypeNone case in > ControlSerializer::load() to catch ommissions when adding new control > types. > s/ommissions/omissions/ This sounds good. Does it cause any issue with no default case found? In fact looking again, Of course this now means that the compiler will warn us here if an option is not handled by the case statement. Excellent. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/control_serializer.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index 5537c5466025..e78fca88cfea 100644 > --- a/src/libcamera/control_serializer.cpp > +++ b/src/libcamera/control_serializer.cpp > @@ -361,7 +361,7 @@ ControlValue ControlSerializer::load<ControlValue>(ControlType type, > return ControlValue(value); > } > > - default: > + case ControlTypeNone: > return ControlValue(); > } > } >
Hi Kieran, On Mon, Mar 02, 2020 at 03:35:23PM +0000, Kieran Bingham wrote: > On 01/03/2020 16:02, Laurent Pinchart wrote: > > Replace the default case with an explicit ControlTypeNone case in > > ControlSerializer::load() to catch ommissions when adding new control > > types. > > s/ommissions/omissions/ > > This sounds good. Does it cause any issue with no default case found? > > In fact looking again, Of course this now means that the compiler will > warn us here if an option is not handled by the case statement. But some gcc version still reports that the function has an exit path with a return statement :-( I'll have to add both. > Excellent. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/control_serializer.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > > index 5537c5466025..e78fca88cfea 100644 > > --- a/src/libcamera/control_serializer.cpp > > +++ b/src/libcamera/control_serializer.cpp > > @@ -361,7 +361,7 @@ ControlValue ControlSerializer::load<ControlValue>(ControlType type, > > return ControlValue(value); > > } > > > > - default: > > + case ControlTypeNone: > > return ControlValue(); > > } > > }
Hi Laurent, On 02/03/2020 16:41, Laurent Pinchart wrote: > Hi Kieran, > > On Mon, Mar 02, 2020 at 03:35:23PM +0000, Kieran Bingham wrote: >> On 01/03/2020 16:02, Laurent Pinchart wrote: >>> Replace the default case with an explicit ControlTypeNone case in >>> ControlSerializer::load() to catch ommissions when adding new control >>> types. >> >> s/ommissions/omissions/ >> >> This sounds good. Does it cause any issue with no default case found? >> >> In fact looking again, Of course this now means that the compiler will >> warn us here if an option is not handled by the case statement. > > But some gcc version still reports that the function has an exit path > with a return statement :-( I'll have to add both. Does adding both remove the benefit of having the compiler warn us when we add new enum types without adding them to the case statement? If not then that's fine. > >> Excellent. >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> src/libcamera/control_serializer.cpp | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp >>> index 5537c5466025..e78fca88cfea 100644 >>> --- a/src/libcamera/control_serializer.cpp >>> +++ b/src/libcamera/control_serializer.cpp >>> @@ -361,7 +361,7 @@ ControlValue ControlSerializer::load<ControlValue>(ControlType type, >>> return ControlValue(value); >>> } >>> >>> - default: >>> + case ControlTypeNone: >>> return ControlValue(); >>> } >>> } >
Hi Kieran, On Mon, Mar 02, 2020 at 10:36:36PM +0000, Kieran Bingham wrote: > On 02/03/2020 16:41, Laurent Pinchart wrote: > > On Mon, Mar 02, 2020 at 03:35:23PM +0000, Kieran Bingham wrote: > >> On 01/03/2020 16:02, Laurent Pinchart wrote: > >>> Replace the default case with an explicit ControlTypeNone case in > >>> ControlSerializer::load() to catch ommissions when adding new control > >>> types. > >> > >> s/ommissions/omissions/ > >> > >> This sounds good. Does it cause any issue with no default case found? > >> > >> In fact looking again, Of course this now means that the compiler will > >> warn us here if an option is not handled by the case statement. > > > > But some gcc version still reports that the function has an exit path > > with a return statement :-( I'll have to add both. > > Does adding both remove the benefit of having the compiler warn us when > we add new enum types without adding them to the case statement? > > If not then that's fine. The benefit stays :-) I've just tested it. > >> Excellent. > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> src/libcamera/control_serializer.cpp | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > >>> index 5537c5466025..e78fca88cfea 100644 > >>> --- a/src/libcamera/control_serializer.cpp > >>> +++ b/src/libcamera/control_serializer.cpp > >>> @@ -361,7 +361,7 @@ ControlValue ControlSerializer::load<ControlValue>(ControlType type, > >>> return ControlValue(value); > >>> } > >>> > >>> - default: > >>> + case ControlTypeNone: > >>> return ControlValue(); > >>> } > >>> }
diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp index 5537c5466025..e78fca88cfea 100644 --- a/src/libcamera/control_serializer.cpp +++ b/src/libcamera/control_serializer.cpp @@ -361,7 +361,7 @@ ControlValue ControlSerializer::load<ControlValue>(ControlType type, return ControlValue(value); } - default: + case ControlTypeNone: return ControlValue(); } }
Replace the default case with an explicit ControlTypeNone case in ControlSerializer::load() to catch ommissions when adding new control types. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/control_serializer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)