Message ID | 20191126233620.1695316-11-niklas.soderlund@ragnatech.se |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Niklas, On Wed, Nov 27, 2019 at 12:36:00AM +0100, Niklas Söderlund wrote: > There is no need to have a private helper function to access a private > data member when a friend statement is needed anyhow. Remove the helper > function to simplify the code and make it clear that a private member of > Buffer is accessed. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > include/libcamera/buffer.h | 2 -- > src/libcamera/buffer.cpp | 8 -------- > src/libcamera/request.cpp | 4 ++-- > 3 files changed, 2 insertions(+), 12 deletions(-) > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > index afcef805801a43b5..d6db6138ca11d5fe 100644 > --- a/include/libcamera/buffer.h > +++ b/include/libcamera/buffer.h > @@ -135,8 +135,6 @@ private: > > void cancel(); > > - void setRequest(Request *request) { request_ = request; } > - > unsigned int index_; > std::array<int, 3> dmabuf_; > BufferMemory *mem_; > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > index 82b4799a2510d02f..7043345c3f3207cd 100644 > --- a/src/libcamera/buffer.cpp > +++ b/src/libcamera/buffer.cpp > @@ -462,7 +462,6 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata) > * > * \return The Request the Buffer belongs to, or nullptr if the buffer is > * either completed or not associated with a request > - * \sa Buffer::setRequest() > */ > > /** > @@ -489,13 +488,6 @@ void Buffer::cancel() > status_ = BufferCancelled; > } > > -/** > - * \fn Buffer::setRequest() > - * \brief Set the request this buffer belongs to > - * > - * The intended callers are Request::prepare() and Request::completeBuffer(). > - */ > - > /** > * \class FrameBuffer > * \brief A buffer handle and dynamic metadata > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index c14ed1a4d3ce55d0..c2854dc2e8caab2e 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -220,7 +220,7 @@ int Request::prepare() > > for (auto const &pair : bufferMap_) { > Buffer *buffer = pair.second; > - buffer->setRequest(this); > + buffer->request_ = this; > pending_.insert(buffer); > } > > @@ -258,7 +258,7 @@ bool Request::completeBuffer(Buffer *buffer) > int ret = pending_.erase(buffer); > ASSERT(ret == 1); > > - buffer->setRequest(nullptr); > + buffer->request_ = nullptr; > > if (buffer->status() == Buffer::BufferCancelled) > cancelled_ = true; > -- > 2.24.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. On Wed, Nov 27, 2019 at 12:36:00AM +0100, Niklas Söderlund wrote: > There is no need to have a private helper function to access a private > data member when a friend statement is needed anyhow. Remove the helper > function to simplify the code and make it clear that a private member of > Buffer is accessed. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/buffer.h | 2 -- > src/libcamera/buffer.cpp | 8 -------- > src/libcamera/request.cpp | 4 ++-- > 3 files changed, 2 insertions(+), 12 deletions(-) > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > index afcef805801a43b5..d6db6138ca11d5fe 100644 > --- a/include/libcamera/buffer.h > +++ b/include/libcamera/buffer.h > @@ -135,8 +135,6 @@ private: > > void cancel(); > > - void setRequest(Request *request) { request_ = request; } > - > unsigned int index_; > std::array<int, 3> dmabuf_; > BufferMemory *mem_; > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > index 82b4799a2510d02f..7043345c3f3207cd 100644 > --- a/src/libcamera/buffer.cpp > +++ b/src/libcamera/buffer.cpp > @@ -462,7 +462,6 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata) > * > * \return The Request the Buffer belongs to, or nullptr if the buffer is > * either completed or not associated with a request > - * \sa Buffer::setRequest() > */ > > /** > @@ -489,13 +488,6 @@ void Buffer::cancel() > status_ = BufferCancelled; > } > > -/** > - * \fn Buffer::setRequest() > - * \brief Set the request this buffer belongs to > - * > - * The intended callers are Request::prepare() and Request::completeBuffer(). > - */ I'm fine with the change overall, but I'm concerned about losing this part of the documentation. Could you document the private request_ member to keep track of this ? /** * \var Buffer::request_ * \brief The request this buffer belongs to * * This member is intended to be set by Request::prepare() and * Request::completeBuffer(). */ Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > - > /** > * \class FrameBuffer > * \brief A buffer handle and dynamic metadata > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index c14ed1a4d3ce55d0..c2854dc2e8caab2e 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -220,7 +220,7 @@ int Request::prepare() > > for (auto const &pair : bufferMap_) { > Buffer *buffer = pair.second; > - buffer->setRequest(this); > + buffer->request_ = this; > pending_.insert(buffer); > } > > @@ -258,7 +258,7 @@ bool Request::completeBuffer(Buffer *buffer) > int ret = pending_.erase(buffer); > ASSERT(ret == 1); > > - buffer->setRequest(nullptr); > + buffer->request_ = nullptr; > > if (buffer->status() == Buffer::BufferCancelled) > cancelled_ = true;
diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h index afcef805801a43b5..d6db6138ca11d5fe 100644 --- a/include/libcamera/buffer.h +++ b/include/libcamera/buffer.h @@ -135,8 +135,6 @@ private: void cancel(); - void setRequest(Request *request) { request_ = request; } - unsigned int index_; std::array<int, 3> dmabuf_; BufferMemory *mem_; diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp index 82b4799a2510d02f..7043345c3f3207cd 100644 --- a/src/libcamera/buffer.cpp +++ b/src/libcamera/buffer.cpp @@ -462,7 +462,6 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata) * * \return The Request the Buffer belongs to, or nullptr if the buffer is * either completed or not associated with a request - * \sa Buffer::setRequest() */ /** @@ -489,13 +488,6 @@ void Buffer::cancel() status_ = BufferCancelled; } -/** - * \fn Buffer::setRequest() - * \brief Set the request this buffer belongs to - * - * The intended callers are Request::prepare() and Request::completeBuffer(). - */ - /** * \class FrameBuffer * \brief A buffer handle and dynamic metadata diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index c14ed1a4d3ce55d0..c2854dc2e8caab2e 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -220,7 +220,7 @@ int Request::prepare() for (auto const &pair : bufferMap_) { Buffer *buffer = pair.second; - buffer->setRequest(this); + buffer->request_ = this; pending_.insert(buffer); } @@ -258,7 +258,7 @@ bool Request::completeBuffer(Buffer *buffer) int ret = pending_.erase(buffer); ASSERT(ret == 1); - buffer->setRequest(nullptr); + buffer->request_ = nullptr; if (buffer->status() == Buffer::BufferCancelled) cancelled_ = true;
There is no need to have a private helper function to access a private data member when a friend statement is needed anyhow. Remove the helper function to simplify the code and make it clear that a private member of Buffer is accessed. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/buffer.h | 2 -- src/libcamera/buffer.cpp | 8 -------- src/libcamera/request.cpp | 4 ++-- 3 files changed, 2 insertions(+), 12 deletions(-)