Product: SonarCloud
Language: C++
Rule: S1448 Class has 36 methods, which is greater than the 35 authorized. Split it into smaller classes.
I managed to trigger this rule by explicitly deleting the copy/move constructors and copy/move assignment operators in an existing class, which pushed it over the 35 methods limit. I consider this a false-positive, since the methods were not being added to the class but rather they were implicitly-created methods being deleted.
To reproduce:
Set up a class with 32 methods.
Add explicit deletions for the copy constructor, copy-assignment operator, move constructor and move-assignment operator.
Rule will be triggered.
Similar behavior exists with defaulting the members, which also feels like a false-positive due to these methods already existing even if they are not explicitly defaulted.
class Test
{
public:
/* can replace "default" with "delete" below and still trigger S1448 */
Test(const Test&) = delete;
Test& operator=(const Test&) = delete;
Test(Test&&) = delete;
Test& operator=(Test&&) = delete;
int method1() const;
int method2() const;
/* ...and so on up to */
int method32() const;
}
Thanks for taking the time to provide the feedback. I can see the point you are making that default and deleted functions should be excluded in S1448. However, I think that the current behavior is reasonable for the following reasons:
The rule is aiming to calculate a number that measures the complexity of a class, in order to suggest when the class needs to be refactored.
Most classes should follow the rule of zero (least complexity). Explicitly defaulted and deleted special member functions count as additional complexity that needs to be carefully understood when reading the class. For example:
Explicitly deleted copy-constructor or move-constructors are an important part of the contract of the class. Furthermore, deleting each involved special member function manually in a class is error-prone as it can violate the rule of five (S3624).
A user-declared copy constructor (even when defaulted) inhibits the generation of a move constructor, which may result in a hidden performance cost.
Even when considering C++20 defaulted comparison operators: I think that these should be counted as complexity as well. It seems fair to me that the default set of supported operators one gets by defaulting the three-way comparison operator counts as a single method in terms of complexity, but picking individual operators and skipping others explicitly counts as more complexity.
The argument I am trying to make is that defaulted and deleted methods contribute to the complexity of the class as there are some nuances that need to be considered by the reader. Also, the usual techniques of tackling complexities apply here: When one has to default and/or delete the same member functions in many classes, then this can be extracted into separate reusable classes that can be inherited or used as members where relevant.
I disagree here. C++ unfortunately makes it far too easy to copy data structures (vectors, maps and the like). Rust gets around this problem by distinguishing between Copy and Clone, but for C++ making classes non-copyable by deleting the copy constructor and copy assignment operator is a useful defence against accidental resource allocations. Often classes don’t actually need to be copyable at all. Deleted copy constructors (and then defaulted move constructor/move assignment operator to get these members back) are in my opinion a simplification rather than additional complexity.
Hi @ajtribick, thanks for sharing your perspective, this is an interesting comparison. If I understand correctly, you are trying to protect against accidental copies by deleting the copy constructor/assignment, and defaulting move constructors/assignments only.
In that case, I would consider having the deleted/defaulted member functions separated in a base class (possibly using private inheritance). In my opinion, this would help:
Make it clear to a newcomer that this is an important concept in my project. This is where I would document the rationale behind the decision of forbidding copies for instance.
Avoid repeating the same logic in each applicable class in the project. This would be less error-prone and easier to quickly understand when reading the class → less complexity.
I would love to hear your input about this option, and if you have reasons to prefer writing those functions in the relevant classes directly.
Hope this helps and thanks again for sharing your views with us!
