How PVS-Studio prevents rash code changes, example N4

Feb 18 2022

Author: Andrey Karpov

If you regularly use a static code analyzer, you can save time on guessing why the new code doesn't work as planned. Let's look at another interesting error — the function broke during refactoring, and no one noticed that. No one — except for PVS-Studio that can automatically scan the project and email the report to us.

0924_Blender_prevents_rash_code_changes_N4/image1.png

It's the fourth small note illustrating how quickly PVS-Studio finds errors in the new code. I thought of taking a break from it. But when I saw PVS-Studio's report about Blender in my emails, I discarded this thought. Let me show you another error, simple and beautiful at the same time.

Once upon a time there was code that processed a vector of values. It prevented values from going beyond a certain range.

#define CLAMP(a, b, c) \
  { \
    if ((a) < (b)) { \
      (a) = (b); \
    } \
    else if ((a) > (c)) { \
      (a) = (c); \
    } \
  } \
  (void)0

template <typename T> inline T
clamp(const T &a, const bT &min_v, const bT &max_v)
{
  T result = a;
  for (int i = 0; i < T::type_length; i++) {
    CLAMP(result[i], min_v, max_v);
  }
  return result;
}

Everything was good. And then the developer decided to abandon the custom CLAMP macro and use the standard std::clamp function. And the commit that supposed to make the code better looked like this:

template <typename T, int Size>
inline vec_base<T, Size>
  clamp(const vec_base<T, Size> &a, const T &min, const T &max)
{
  vec_base<T, Size> result = a;
  for (int i = 0; i < Size; i++) {
    std::clamp(result[i], min, max);
  }
  return result;
}

It seems like the developer was in a hurry. Do you see the error? Maybe yes, maybe no. Anyway, the developer who wrote the code, didn't notice that it was broken.

But the all-seeing PVS-Studio static analyzer warns us immediately:

[CWE-252] V530: The return value of function 'clamp' is required to be utilized. BLI_math_vector.hh 88

The point being — the std::clamp function doesn't change the value of the element in the container:

template <class T>
constexpr const T&
clamp( const T& v, const T& lo, const T& hi );

The CLAMP macro used to change the value, but the standard function did not. Now the code is broken and is waiting for someone to notice an error and look for its cause. With PVS-Studio, the developers could have found and fixed this error at the code writing stage. Using static analysis regularly, you can save your time and resources.

Note. By the way, there's another incorrect use of std::clamp in the code.

The correct version of code:

template <typename T, int Size>
inline vec_base<T, Size>
clamp(const vec_base<T, Size> &a, const T &min, const T &max)
{
  vec_base<T, Size> result = a;
  for (int i = 0; i < Size; i++) {
    result[i] = std::clamp(result[i], min, max);
  }
  return result;
}

Thank you for your time. And drop by to read about the top 10 bugs found in C++ open-source projects in 2021.

#Cpp