False positive on cpp:S3230 when using delegating constructors

Environment:

SonarLint 5.3.0.36775 SonarSource
Clion 2021.1.3
Ubuntu 20.04.3 LTS
False positive on code, suggesting refactoring to a version which would be ill-formed.
When using delegating contructors, further re-assignment of fields is only possible in the constructor body, not in the initialization list.
SonarLint tells “Do not assign data members in the constructor Initialize data member in an initialization list”

#include <vector>

class Interpolator
{
public:
  explicit Interpolator(std::vector<std::pair<double, double>> coord) : c(std::move(coord))
  {
    /* pretend there is some convoluted check for content of coord here */
  }

  Interpolator(double minX, double maxX, const std::vector<double>& yValues)
      : Interpolator(implyCoords(minX, maxX, yValues))
  {
    isEquidistant = true; // false positive on this line
  }

private:
  using Coordinates = std::vector<std::pair<double, double>>;
  Coordinates c;
  bool isEquidistant = false;

  static Coordinates implyCoords(double , double , const std::vector<double>& )
  {
    return {};  // pretend there is some real logic here to calculate coordinates for equidistant interpolator
  }
};

Hello @shojtsy,

This rule is trying to argue that you should always initialize all member data in the member initializer list or in the class-default member initializer.

It is true that in your example isEquidistant cannot be initialized in the same constructor, the rule and the message doesn’t claim that, but still It can and should be initialized in a constructor initializer list according to the rule. This can be achieved by adding an overload or default arguments(depending on your coding standard).

Do you agree?

Thanks,

Hello Abbas,

Adding a bool isEquidistant argument to the publicly accessible constructor sounds like a fragile API, as it would be easy to misuse: pass non-equidistant points and claim they are equidistant, which would break the logic later on when you use them.
But it seems workable in my initial example to introduce a third overload of the constructor, make that private, and have both public constructors delegate to that, and only have the third constructor initialize fields ever in an initializer list.
I am however not convinced that the mental cost of one more indirection and boilerplate code necessary makes this a better quality solution.

class Interpolator
{
public:
  explicit Interpolator(std::vector<std::pair<double, double>> coord) :
      Interpolator(std::move(coord), false)                                                 
  {}

  Interpolator(double minX, double maxX, const std::vector<double>& yValues)
      : Interpolator(implyCoords(minX, maxX, yValues), true)
  {}

private:
  using Coordinates = std::vector<std::pair<double, double>>;
  Coordinates c;
  bool isEquidistant;

  static Coordinates implyCoords(double, double, const std::vector<double>&)
  {
    return {};  // pretend there is some real logic here to calculate coordinates for equidistant interpolator
  }

  Interpolator(std::vector<std::pair<double, double>> coord, bool isEquidistant_)
      : c(std::move(coord)), isEquidistant(isEquidistant_)
  {
    /* pretend there is some convoluted check for content of coord here */
  }
};

@shojtsy,

Yes, this is what the rule is advocating for.

It is arguable depending on how you look at it. On one side there is more boilerplate code, on the other side, it is clear how isEquidistant is initialized.
Before it was spread in three places: class-default member initializer, constructor body, and possibly, constructors initializer list. After the change, isEquidistant is in one place.

Thanks,

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.