| Message ID | 20200806125330.2983325-2-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Ohh another series making things that should be const ... const ... ;-) This makes me happy too. On 06/08/2020 13:53, Niklas Söderlund wrote: > There is nothing blocking cameraData() from being a constant operation. > The assert already enforces that a std::map::at() operation would always > succeed. Switch to using at() and mark the method as a const operation. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Sounds good to me! Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/internal/pipeline_handler.h | 2 +- > src/libcamera/pipeline_handler.cpp | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 22e629a8401d1e4e..d5321ef56df358d0 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -96,7 +96,7 @@ protected: > > virtual int queueRequestDevice(Camera *camera, Request *request) = 0; > > - CameraData *cameraData(const Camera *camera); > + CameraData *cameraData(const Camera *camera) const; > > CameraManager *manager_; > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index ccd45edc847b9e3f..bc9c1d4b09c5437d 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -595,10 +595,10 @@ void PipelineHandler::disconnect() > * valid until the pipeline handler is destroyed. It shall not be deleted > * manually by the caller. > */ > -CameraData *PipelineHandler::cameraData(const Camera *camera) > +CameraData *PipelineHandler::cameraData(const Camera *camera) const > { > ASSERT(cameraData_.count(camera)); > - return cameraData_[camera].get(); > + return cameraData_.at(camera).get(); > } > > /** >
Hi Niklas, Thank you for the patch. On Thu, Aug 06, 2020 at 02:53:28PM +0200, Niklas Söderlund wrote: > There is nothing blocking cameraData() from being a constant operation. > The assert already enforces that a std::map::at() operation would always > succeed. Switch to using at() and mark the method as a const operation. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/internal/pipeline_handler.h | 2 +- > src/libcamera/pipeline_handler.cpp | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 22e629a8401d1e4e..d5321ef56df358d0 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -96,7 +96,7 @@ protected: > > virtual int queueRequestDevice(Camera *camera, Request *request) = 0; > > - CameraData *cameraData(const Camera *camera); > + CameraData *cameraData(const Camera *camera) const; CameraData holds part of the pipeline state. As such, returning a non-const pointer from a const function, while not breaking compilation, conceptually breaks const-correctness. Would it be enough for your use case to have both CameraData *cameraData(const Camera *camera); const CameraData *cameraData(const Camera *camera) const; ? > > CameraManager *manager_; > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index ccd45edc847b9e3f..bc9c1d4b09c5437d 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -595,10 +595,10 @@ void PipelineHandler::disconnect() > * valid until the pipeline handler is destroyed. It shall not be deleted > * manually by the caller. > */ > -CameraData *PipelineHandler::cameraData(const Camera *camera) > +CameraData *PipelineHandler::cameraData(const Camera *camera) const > { > ASSERT(cameraData_.count(camera)); > - return cameraData_[camera].get(); > + return cameraData_.at(camera).get(); > } > > /**
Hi Laurent, Thanks for your feedback. On 2020-08-14 12:35:32 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Thu, Aug 06, 2020 at 02:53:28PM +0200, Niklas Söderlund wrote: > > There is nothing blocking cameraData() from being a constant operation. > > The assert already enforces that a std::map::at() operation would always > > succeed. Switch to using at() and mark the method as a const operation. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > include/libcamera/internal/pipeline_handler.h | 2 +- > > src/libcamera/pipeline_handler.cpp | 4 ++-- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > index 22e629a8401d1e4e..d5321ef56df358d0 100644 > > --- a/include/libcamera/internal/pipeline_handler.h > > +++ b/include/libcamera/internal/pipeline_handler.h > > @@ -96,7 +96,7 @@ protected: > > > > virtual int queueRequestDevice(Camera *camera, Request *request) = 0; > > > > - CameraData *cameraData(const Camera *camera); > > + CameraData *cameraData(const Camera *camera) const; > > CameraData holds part of the pipeline state. As such, returning a > non-const pointer from a const function, while not breaking compilation, > conceptually breaks const-correctness. > > Would it be enough for your use case to have both > > CameraData *cameraData(const Camera *camera); > const CameraData *cameraData(const Camera *camera) const; > > ? Thanks for spotting this, not sure how this brain fart happened. Your proposed change solves my issue in a nicer and foremost correct way, will send a v2. > > > > > CameraManager *manager_; > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index ccd45edc847b9e3f..bc9c1d4b09c5437d 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -595,10 +595,10 @@ void PipelineHandler::disconnect() > > * valid until the pipeline handler is destroyed. It shall not be deleted > > * manually by the caller. > > */ > > -CameraData *PipelineHandler::cameraData(const Camera *camera) > > +CameraData *PipelineHandler::cameraData(const Camera *camera) const > > { > > ASSERT(cameraData_.count(camera)); > > - return cameraData_[camera].get(); > > + return cameraData_.at(camera).get(); > > } > > > > /** > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index 22e629a8401d1e4e..d5321ef56df358d0 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -96,7 +96,7 @@ protected: virtual int queueRequestDevice(Camera *camera, Request *request) = 0; - CameraData *cameraData(const Camera *camera); + CameraData *cameraData(const Camera *camera) const; CameraManager *manager_; diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index ccd45edc847b9e3f..bc9c1d4b09c5437d 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -595,10 +595,10 @@ void PipelineHandler::disconnect() * valid until the pipeline handler is destroyed. It shall not be deleted * manually by the caller. */ -CameraData *PipelineHandler::cameraData(const Camera *camera) +CameraData *PipelineHandler::cameraData(const Camera *camera) const { ASSERT(cameraData_.count(camera)); - return cameraData_[camera].get(); + return cameraData_.at(camera).get(); } /**
There is nothing blocking cameraData() from being a constant operation. The assert already enforces that a std::map::at() operation would always succeed. Switch to using at() and mark the method as a const operation. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/internal/pipeline_handler.h | 2 +- src/libcamera/pipeline_handler.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)