| Message ID | 20240404084657.353464-6-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2024-04-04 09:46:42) Potential expansion to add (while committing??) The SharedMemObject has been imported directly into the libcamera internal components. Adapt the SIZE constant of the class to match the libcamera coding style. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> That said, don't we prefix constants with a k? kSize ? Or maybe it's not quite globally constant as it's only constant to the class instantiation? And as it's a class variable should it be postfixed with an underscore? static constexpr kSize_ = sizeof(T); ? > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > include/libcamera/internal/shared_mem_object.h | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h > index 98636b44..8f2e7120 100644 > --- a/include/libcamera/internal/shared_mem_object.h > +++ b/include/libcamera/internal/shared_mem_object.h > @@ -23,7 +23,7 @@ template<class T> > class SharedMemObject > { > public: > - static constexpr std::size_t SIZE = sizeof(T); > + static constexpr std::size_t size = sizeof(T); > > SharedMemObject() > : obj_(nullptr) > @@ -45,11 +45,11 @@ public: > if (!fd_.isValid()) > return; > > - ret = ftruncate(fd_.get(), SIZE); > + ret = ftruncate(fd_.get(), size); > if (ret < 0) > return; > > - mem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, > + mem = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED, > fd_.get(), 0); > if (mem == MAP_FAILED) > return; > @@ -69,7 +69,7 @@ public: > { > if (obj_) { > obj_->~T(); > - munmap(obj_, SIZE); > + munmap(obj_, size); > } > } > > -- > 2.42.0 >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2024-04-04 09:46:42) > > Potential expansion to add (while committing??) > > The SharedMemObject has been imported directly into the libcamera > internal components. Adapt the SIZE constant of the class to match the > libcamera coding style. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > That said, don't we prefix constants with a k? kSize ? Or maybe it's not > quite globally constant as it's only constant to the class > instantiation? > > And as it's a class variable should it be postfixed with an underscore? > > static constexpr kSize_ = sizeof(T); ? Right, I'll fix it if we decide for v8, otherwise I can fix it in a followup patch. Thanks, Milan >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> include/libcamera/internal/shared_mem_object.h | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h >> index 98636b44..8f2e7120 100644 >> --- a/include/libcamera/internal/shared_mem_object.h >> +++ b/include/libcamera/internal/shared_mem_object.h >> @@ -23,7 +23,7 @@ template<class T> >> class SharedMemObject >> { >> public: >> - static constexpr std::size_t SIZE = sizeof(T); >> + static constexpr std::size_t size = sizeof(T); >> >> SharedMemObject() >> : obj_(nullptr) >> @@ -45,11 +45,11 @@ public: >> if (!fd_.isValid()) >> return; >> >> - ret = ftruncate(fd_.get(), SIZE); >> + ret = ftruncate(fd_.get(), size); >> if (ret < 0) >> return; >> >> - mem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, >> + mem = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED, >> fd_.get(), 0); >> if (mem == MAP_FAILED) >> return; >> @@ -69,7 +69,7 @@ public: >> { >> if (obj_) { >> obj_->~T(); >> - munmap(obj_, SIZE); >> + munmap(obj_, size); >> } >> } >> >> -- >> 2.42.0 >>
Hello, On Mon, Apr 08, 2024 at 05:40:33PM +0200, Milan Zamazal wrote: > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > > > Quoting Milan Zamazal (2024-04-04 09:46:42) > > > > Potential expansion to add (while committing??) > > > > The SharedMemObject has been imported directly into the libcamera > > internal components. Adapt the SIZE constant of the class to match the > > libcamera coding style. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > That said, don't we prefix constants with a k? kSize ? Or maybe it's not > > quite globally constant as it's only constant to the class > > instantiation? > > > > And as it's a class variable should it be postfixed with an underscore? > > > > static constexpr kSize_ = sizeof(T); ? It should be prefixed with a k, yes. As for the trailing underscore, I don't think we use it for constants. With kSize, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Right, I'll fix it if we decide for v8, otherwise I can fix it in a followup > patch. There are minor review comments on other patches, so I think a final v8 will be needed. I'm worried I would be messing up something if I applied all the small changes myself. > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> include/libcamera/internal/shared_mem_object.h | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h > >> index 98636b44..8f2e7120 100644 > >> --- a/include/libcamera/internal/shared_mem_object.h > >> +++ b/include/libcamera/internal/shared_mem_object.h > >> @@ -23,7 +23,7 @@ template<class T> > >> class SharedMemObject > >> { > >> public: > >> - static constexpr std::size_t SIZE = sizeof(T); > >> + static constexpr std::size_t size = sizeof(T); > >> > >> SharedMemObject() > >> : obj_(nullptr) > >> @@ -45,11 +45,11 @@ public: > >> if (!fd_.isValid()) > >> return; > >> > >> - ret = ftruncate(fd_.get(), SIZE); > >> + ret = ftruncate(fd_.get(), size); > >> if (ret < 0) > >> return; > >> > >> - mem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, > >> + mem = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED, > >> fd_.get(), 0); > >> if (mem == MAP_FAILED) > >> return; > >> @@ -69,7 +69,7 @@ public: > >> { > >> if (obj_) { > >> obj_->~T(); > >> - munmap(obj_, SIZE); > >> + munmap(obj_, size); > >> } > >> } > >> >
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hello, > > On Mon, Apr 08, 2024 at 05:40:33PM +0200, Milan Zamazal wrote: >> Kieran Bingham <kieran.bingham@ideasonboard.com> writes: >> >> > Quoting Milan Zamazal (2024-04-04 09:46:42) >> > >> > Potential expansion to add (while committing??) >> > >> > The SharedMemObject has been imported directly into the libcamera >> > internal components. Adapt the SIZE constant of the class to match the >> > libcamera coding style. >> > >> > >> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> > >> > >> > That said, don't we prefix constants with a k? kSize ? Or maybe it's not >> > quite globally constant as it's only constant to the class >> > instantiation? >> > >> > And as it's a class variable should it be postfixed with an underscore? >> > >> > static constexpr kSize_ = sizeof(T); ? > > It should be prefixed with a k, yes. As for the trailing underscore, I > don't think we use it for constants. > > With kSize, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Right, I'll fix it if we decide for v8, otherwise I can fix it in a followup >> patch. > > There are minor review comments on other patches, so I think a final v8 > will be needed. I'm worried I would be messing up something if I applied > all the small changes myself. Hi Laurent, OK, I'll prepare v8. Regards, Milan >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> >> --- >> >> include/libcamera/internal/shared_mem_object.h | 8 ++++---- >> >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h >> >> index 98636b44..8f2e7120 100644 >> >> --- a/include/libcamera/internal/shared_mem_object.h >> >> +++ b/include/libcamera/internal/shared_mem_object.h >> >> @@ -23,7 +23,7 @@ template<class T> >> >> class SharedMemObject >> >> { >> >> public: >> >> - static constexpr std::size_t SIZE = sizeof(T); >> >> + static constexpr std::size_t size = sizeof(T); >> >> >> >> SharedMemObject() >> >> : obj_(nullptr) >> >> @@ -45,11 +45,11 @@ public: >> >> if (!fd_.isValid()) >> >> return; >> >> >> >> - ret = ftruncate(fd_.get(), SIZE); >> >> + ret = ftruncate(fd_.get(), size); >> >> if (ret < 0) >> >> return; >> >> >> >> - mem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, >> >> + mem = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED, >> >> fd_.get(), 0); >> >> if (mem == MAP_FAILED) >> >> return; >> >> @@ -69,7 +69,7 @@ public: >> >> { >> >> if (obj_) { >> >> obj_->~T(); >> >> - munmap(obj_, SIZE); >> >> + munmap(obj_, size); >> >> } >> >> } >> >> >>
diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h index 98636b44..8f2e7120 100644 --- a/include/libcamera/internal/shared_mem_object.h +++ b/include/libcamera/internal/shared_mem_object.h @@ -23,7 +23,7 @@ template<class T> class SharedMemObject { public: - static constexpr std::size_t SIZE = sizeof(T); + static constexpr std::size_t size = sizeof(T); SharedMemObject() : obj_(nullptr) @@ -45,11 +45,11 @@ public: if (!fd_.isValid()) return; - ret = ftruncate(fd_.get(), SIZE); + ret = ftruncate(fd_.get(), size); if (ret < 0) return; - mem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, + mem = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd_.get(), 0); if (mem == MAP_FAILED) return; @@ -69,7 +69,7 @@ public: { if (obj_) { obj_->~T(); - munmap(obj_, SIZE); + munmap(obj_, size); } }
Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- include/libcamera/internal/shared_mem_object.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)