| Message ID | 20201022081731.36217-4-tomi.valkeinen@iki.fi |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi, Thank you for the patch. On Thu, Oct 22, 2020 at 11:17:31AM +0300, Tomi Valkeinen wrote: > These are quick hacks to silence clang-tidy warnings. The purpose of > this patch is just to highlight the warnings on the mailing list, and > also, if you want to try to fix some of the warnings, it may be nice to > have all the rest suppressed. > > Afaics, the thread->moveObject(this) warning is a false positive. > Probably some of the others are too. > > Note that I used assert() there in a few places, just after ASSERT(). > The reason being, ASSERT() just prints an error, while assert() aborts, > so clang-tidy takes the assert() seriously. LOG(Fatal) aborts too. Could this be fixed with annotate ASSERT() with [[noreturn]] ? Something along the lines of (untested) diff --git a/include/libcamera/internal/log.h b/include/libcamera/internal/log.h index 4b10087a4718..a9fc5526a971 100644 --- a/include/libcamera/internal/log.h +++ b/include/libcamera/internal/log.h @@ -117,9 +117,14 @@ LogMessage _log(const char *file, unsigned int line, #endif /* __DOXYGEN__ */ #ifndef NDEBUG +static inline [[noreturn]] void _assert(const char *message) +{ + LOG(Fatal) << message; +} + #define ASSERT(condition) static_cast<void>(({ \ if (!(condition)) \ - LOG(Fatal) << "assertion \"" #condition "\" failed"; \ + _assert("assertion \"" #condition "\" failed"); \ })) #else #define ASSERT(condition) static_cast<void>(false && (condition)) > Not for merging. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi> > --- > src/ipa/raspberrypi/controller/histogram.cpp | 2 +- > src/libcamera/object.cpp | 3 ++- > src/libcamera/pipeline/ipu3/ipu3.cpp | 1 + > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++ > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 ++ > src/libcamera/v4l2_device.cpp | 1 - > src/libcamera/v4l2_pixelformat.cpp | 2 +- > 7 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/raspberrypi/controller/histogram.cpp b/src/ipa/raspberrypi/controller/histogram.cpp > index 9916b3ed..ac3f5c48 100644 > --- a/src/ipa/raspberrypi/controller/histogram.cpp > +++ b/src/ipa/raspberrypi/controller/histogram.cpp > @@ -51,7 +51,7 @@ double Histogram::InterQuantileMean(double q_lo, double q_hi) const > double p_lo = Quantile(q_lo); > double p_hi = Quantile(q_hi, (int)p_lo); > double sum_bin_freq = 0, cumul_freq = 0; > - for (double p_next = floor(p_lo) + 1.0; p_next <= ceil(p_hi); > + for (double p_next = floor(p_lo) + 1.0; p_next <= ceil(p_hi); // NOLINT: Variable 'p_next' with floating point type 'double' should not be used as a loop counter [clang-analyzer-security.FloatLoopCounter] > p_lo = p_next, p_next += 1.0) { > int bin = floor(p_lo); > double freq = (cumulative_[bin + 1] - cumulative_[bin]) * > diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp > index cd83c684..1b1775e6 100644 > --- a/src/libcamera/object.cpp > +++ b/src/libcamera/object.cpp > @@ -257,7 +257,8 @@ void Object::moveToThread(Thread *thread) > > notifyThreadMove(); > > - thread->moveObject(this); > + // clang-tidy thinks that the thread can be deleted via DeferredDelete handling in notifyThreadMove() The thread, or 'this' object ? It seems to be a false positive in any case. > + thread->moveObject(this); // NOLINT: warning: Use of memory after it is freed [clang-analyzer-cplusplus.NewDelete] > } > > void Object::notifyThreadMove() > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index af47739d..eac27153 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -508,6 +508,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > * be at least one active stream in the configuration request). > */ > if (!vfCfg) { > + assert(mainCfg); > ret = imgu->configureViewfinder(*mainCfg, &outputFormat); > if (ret) > return ret; > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index aa8d9340..4544d606 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1290,6 +1290,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > /* The buffer must belong to one of our streams. */ > ASSERT(stream); > + assert(stream); > > LOG(RPI, Debug) << "Stream " << stream->name() << " buffer dequeue" > << ", buffer id " << index > @@ -1358,6 +1359,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer) > > /* The buffer must belong to one of our ISP output streams. */ > ASSERT(stream); > + assert(stream); > > LOG(RPI, Debug) << "Stream " << stream->name() << " buffer complete" > << ", buffer id " << index > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index c74a2e9b..be55c32f 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -379,6 +379,8 @@ protected: > if (!info) > LOG(RkISP1, Fatal) << "Frame not known"; This could be replace by ASSERT(). > > + assert(info); > + > /* > * \todo: If parameters are not filled a better method to handle > * the situation than queuing a buffer with unknown content > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 31d4dad0..f747ba79 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -243,7 +243,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > LOG(V4L2, Error) << "Unable to read control " << errorIdx > << ": " << strerror(-ret); > count = errorIdx - 1; > - ret = errorIdx; This could indeed be dropped. > } > > updateControls(&ctrls, v4l2Ctrls, count); > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp > index 03ab085c..354136b7 100644 > --- a/src/libcamera/v4l2_pixelformat.cpp > +++ b/src/libcamera/v4l2_pixelformat.cpp > @@ -163,7 +163,7 @@ std::string V4L2PixelFormat::toString() const > } > > if (fourcc_ & (1 << 31)) > - strcat(ss, "-BE"); > + memcpy(ss + 4, "-BE", 3); What bug does this fix ? > > return ss; > }
diff --git a/src/ipa/raspberrypi/controller/histogram.cpp b/src/ipa/raspberrypi/controller/histogram.cpp index 9916b3ed..ac3f5c48 100644 --- a/src/ipa/raspberrypi/controller/histogram.cpp +++ b/src/ipa/raspberrypi/controller/histogram.cpp @@ -51,7 +51,7 @@ double Histogram::InterQuantileMean(double q_lo, double q_hi) const double p_lo = Quantile(q_lo); double p_hi = Quantile(q_hi, (int)p_lo); double sum_bin_freq = 0, cumul_freq = 0; - for (double p_next = floor(p_lo) + 1.0; p_next <= ceil(p_hi); + for (double p_next = floor(p_lo) + 1.0; p_next <= ceil(p_hi); // NOLINT: Variable 'p_next' with floating point type 'double' should not be used as a loop counter [clang-analyzer-security.FloatLoopCounter] p_lo = p_next, p_next += 1.0) { int bin = floor(p_lo); double freq = (cumulative_[bin + 1] - cumulative_[bin]) * diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp index cd83c684..1b1775e6 100644 --- a/src/libcamera/object.cpp +++ b/src/libcamera/object.cpp @@ -257,7 +257,8 @@ void Object::moveToThread(Thread *thread) notifyThreadMove(); - thread->moveObject(this); + // clang-tidy thinks that the thread can be deleted via DeferredDelete handling in notifyThreadMove() + thread->moveObject(this); // NOLINT: warning: Use of memory after it is freed [clang-analyzer-cplusplus.NewDelete] } void Object::notifyThreadMove() diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index af47739d..eac27153 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -508,6 +508,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) * be at least one active stream in the configuration request). */ if (!vfCfg) { + assert(mainCfg); ret = imgu->configureViewfinder(*mainCfg, &outputFormat); if (ret) return ret; diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index aa8d9340..4544d606 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1290,6 +1290,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) /* The buffer must belong to one of our streams. */ ASSERT(stream); + assert(stream); LOG(RPI, Debug) << "Stream " << stream->name() << " buffer dequeue" << ", buffer id " << index @@ -1358,6 +1359,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer) /* The buffer must belong to one of our ISP output streams. */ ASSERT(stream); + assert(stream); LOG(RPI, Debug) << "Stream " << stream->name() << " buffer complete" << ", buffer id " << index diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index c74a2e9b..be55c32f 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -379,6 +379,8 @@ protected: if (!info) LOG(RkISP1, Fatal) << "Frame not known"; + assert(info); + /* * \todo: If parameters are not filled a better method to handle * the situation than queuing a buffer with unknown content diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 31d4dad0..f747ba79 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -243,7 +243,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) LOG(V4L2, Error) << "Unable to read control " << errorIdx << ": " << strerror(-ret); count = errorIdx - 1; - ret = errorIdx; } updateControls(&ctrls, v4l2Ctrls, count); diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp index 03ab085c..354136b7 100644 --- a/src/libcamera/v4l2_pixelformat.cpp +++ b/src/libcamera/v4l2_pixelformat.cpp @@ -163,7 +163,7 @@ std::string V4L2PixelFormat::toString() const } if (fourcc_ & (1 << 31)) - strcat(ss, "-BE"); + memcpy(ss + 4, "-BE", 3); return ss; }
These are quick hacks to silence clang-tidy warnings. The purpose of this patch is just to highlight the warnings on the mailing list, and also, if you want to try to fix some of the warnings, it may be nice to have all the rest suppressed. Afaics, the thread->moveObject(this) warning is a false positive. Probably some of the others are too. Note that I used assert() there in a few places, just after ASSERT(). The reason being, ASSERT() just prints an error, while assert() aborts, so clang-tidy takes the assert() seriously. Not for merging. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi> --- src/ipa/raspberrypi/controller/histogram.cpp | 2 +- src/libcamera/object.cpp | 3 ++- src/libcamera/pipeline/ipu3/ipu3.cpp | 1 + src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++ src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 ++ src/libcamera/v4l2_device.cpp | 1 - src/libcamera/v4l2_pixelformat.cpp | 2 +- 7 files changed, 9 insertions(+), 4 deletions(-)