Message ID | 20250422124751.30409-2-barnabas.pocze@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Quoting Barnabás Pőcze (2025-04-22 13:47:50) > The current `ControlValue` mechanism does not support arrays > of strings, the assignment in the removed snippet will in fact > trigger an assertion failure in `ControlValue::set()` because > `sizeof(std::string) != ControlValueSize[ControlTypeString]`. > > Fixes: b35f04b3c194 ("cam: capture_script: Support parsing array controls") > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> I like a clear failure - rather than a guaranteed runtime assertion! Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/apps/cam/capture_script.cpp | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp > index e7e69960e..fdf82efc0 100644 > --- a/src/apps/cam/capture_script.cpp > +++ b/src/apps/cam/capture_script.cpp > @@ -578,10 +578,6 @@ ControlValue CaptureScript::parseArrayControl(const ControlId *id, > value = Span<const float>(values.data(), values.size()); > break; > } > - case ControlTypeString: { > - value = Span<const std::string>(repr.data(), repr.size()); > - break; > - } Is it worth keeping case ControlTypeString: here as a fall through with a comment explaining that it can't currently be supported ? (I'm fine with a full removal too - just curious if it's helpful to document explicitly that it is 'unsupported' rather than 'missed' if someone tried to add it.. > default: > std::cerr << "Unsupported control type" << std::endl; > break; > -- > 2.49.0 >
Hi 2025. 05. 01. 10:44 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-04-22 13:47:50) >> The current `ControlValue` mechanism does not support arrays >> of strings, the assignment in the removed snippet will in fact >> trigger an assertion failure in `ControlValue::set()` because >> `sizeof(std::string) != ControlValueSize[ControlTypeString]`. >> >> Fixes: b35f04b3c194 ("cam: capture_script: Support parsing array controls") >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > I like a clear failure - rather than a guaranteed runtime assertion! > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- >> src/apps/cam/capture_script.cpp | 4 ---- >> 1 file changed, 4 deletions(-) >> >> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp >> index e7e69960e..fdf82efc0 100644 >> --- a/src/apps/cam/capture_script.cpp >> +++ b/src/apps/cam/capture_script.cpp >> @@ -578,10 +578,6 @@ ControlValue CaptureScript::parseArrayControl(const ControlId *id, >> value = Span<const float>(values.data(), values.size()); >> break; >> } >> - case ControlTypeString: { >> - value = Span<const std::string>(repr.data(), repr.size()); >> - break; >> - } > > > Is it worth keeping > case ControlTypeString: > > here as a fall through with a comment explaining that it can't currently > be supported ? (I'm fine with a full removal too - just curious if it's > helpful to document explicitly that it is 'unsupported' rather than > 'missed' if someone tried to add it.. I'm hoping to make that a compiler error, so I think it shouldn't be an issue. Regards, Barnabás Pőcze > >> default: >> std::cerr << "Unsupported control type" << std::endl; >> break; >> -- >> 2.49.0 >>
diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp index e7e69960e..fdf82efc0 100644 --- a/src/apps/cam/capture_script.cpp +++ b/src/apps/cam/capture_script.cpp @@ -578,10 +578,6 @@ ControlValue CaptureScript::parseArrayControl(const ControlId *id, value = Span<const float>(values.data(), values.size()); break; } - case ControlTypeString: { - value = Span<const std::string>(repr.data(), repr.size()); - break; - } default: std::cerr << "Unsupported control type" << std::endl; break;
The current `ControlValue` mechanism does not support arrays of strings, the assignment in the removed snippet will in fact trigger an assertion failure in `ControlValue::set()` because `sizeof(std::string) != ControlValueSize[ControlTypeString]`. Fixes: b35f04b3c194 ("cam: capture_script: Support parsing array controls") Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/apps/cam/capture_script.cpp | 4 ---- 1 file changed, 4 deletions(-)