[libcamera-devel] [PATCH 4/4] HACK: suppress clang-tidy warnings

[Laurent Pinchart]

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 at 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;
>  }

-- 
Regards,

Laurent Pinchart