Public API documentation check and deleted functions

[jens-auer]

Hi,

the sonar-cxx version we use complains about undocumented public API for deleted functions, e.g. deleted copy-constructor or assignment operators. We are using an older version, so maybe it is fixed, but I looked at the code in the repo and I think it isn't.

It may also be worth considering default'ed functions (e.g. a default destructor/ constructor) as non-violations, but I am not sure if that is a good idea.

Best wishes,
Jens

PS: code example:
class A
{
public:
A(A const&) = delete:
A& operator=(A const&) = delete;
};

generates violations for undocumented public API.

[lcintrat]

Hi Jens,

The real question is: what is an API?
If we look at Wikipedia:
"In its simplest form, an object API is a description of how objects work in a given object-oriented language"

So, the =delete form describes how the object works, maybe it needs to be documented, e.g:

/**
  * Disabling use of copy constructor because ...
  */
A(A const&) = delete;

Meanwhile, we could add a check and a flag to indicate if it has to be counted as a violation or not.
I don't know what to do for these cases...

Regards,
Ludovic

[guwirth]

Hi,

Before C++11 and delete this declaration was always in the private part of a class. So wondering if we need a switch or declaration can be moved to private section instead?

Regards

Am 07.07.2015 um 22:49 schrieb Ludovic Cintrat [email protected]:

Hi Jens,

The real question is: what is an API?
If we look at Wikipedia:
"In its simplest form, an object API is a description of how objects work in a given object-oriented language"

So, the =delete form describes how the object works, maybe it needs to be documented, e.g:

/**

• Disabling use of copy constructor because ...
  */
  A(A const&) = delete;
  Meanwhile, we could add a check and a flag to indicate if it has to be counted as a violation or not.
  I don't know what to do for these cases...

Regards,
Ludovic

—
Reply to this email directly or view it on GitHub.

[jens-auer]

Hi Ludovic,

I have to disagree on that view. I my opinion, and I don't think the wikipedia definition is a good one, the public API is the contract that an object offers. This includes all the messages it understands, their pre- and postconditions and the object's invariants. This is what forms the public interface. IMHO, the wikipedia expression "how objects work" relates more to the internal implementation of an object, and this is specifically not part of the public API.
From this definition, I would object to document messages which the object does not understand. I usually do not document that a class does not have a default constructor when I define a custom constructor with arguments, and I do not document that a class does not have e.g. comparison operators when I don't implement it.
For the example given, the documentation would probably look like

/**
  * Disabling use of copy constructor because it is non-copyable.
  */
A(A const&) = delete;

This is redundant because the delete explicitly says that.

[jens-auer]

Hi Guenter,

Moving it to private would solve the problem, but I would not like to do so because it seems to be unidiomatic for C++11. I really think that this should be an exception, especially when you consider other usages of deleted functions, e.g. to prevent type promotion. Here I really narrow the class' interface.

[guwirth]

Looking what good old Scott Meyers is saying in 'Effective Modern C++':
"By convention, delete function are declared public, not private. ... will result in better error messages. (Item 11, page 75/76)"

So yes, maybe a good idea to remove deleted functions from public API.

[lcintrat]

Hi Jens,

I agree, the definition of Wikipedia does not suit very well.
However, to declare that a method shall not be implemented is different than using the default behavior.

The explicit use of the =delete overrides the default behavior, and this is done for a good reason: performance requirement or because the boss said to do as is, etc... Otherwise, this is not declared as delete.
In short, I mean that it is a choice (not the default one), and a choice must be argued, so it must be documented. I have to admit this is more an implementation point of view than an API one.

Another point is that if the deleted method is not documented, it may not appear in the final documentation, and the user may wonder why the method is not available (even if he can inspect the header file).

To carry on with the above example:

/**
  * Disabling use of copy constructor to:
  *   - lower memory consumption,
  *   - do not share any internal attribute,
  *   - because of requirement REQ-XXX,
  *   - use method xxx to get a copy (or something else) of this instance,
  *   - because it is a singleton,
  *   - ...
  */
A(A const&) = delete;

IMHO, at the moment, this is more a choice to count =delete method missing documentation as a violation or not. So let's implement it with an activation flag at runtime (defaulting to no violation) :)
I will have a look this weekend I think.

Something that can enlighten us on this point: what is the behavior of Doxygen (or other tools) on such methods?

PS:
Moving the method as private is not the same thing, because it could be called from a method of the class.

[jens-auer]

Hi Ludovic,

I don't think we will converge here. The usual reason to make an object non-copyable is because it contains a non-copyable member. In that case, declaring the copy constructor (and assignment operator) deleted helps the user of the class to get good error messages, but ut does not change the default behavior. If the class has a non-copyable member, the compiler will not generate a copy-constructor, but generate an error when you use the class. The deleted constructor just makes the intent clear. Consider

class A {
   private:
       std::unique_ptr<B> m_ptr;
};

In this case, the compiler will not generate a copy-constructor.

I am tempted to also say that requiring documentation for defaulted methods (copy-constructor, assignment operator, destructor) should also not be a violation because this has precisely defined semantics. In general, I think that if you need to document what these standard functions do, the design is at least questionable and often illformed.
Forcing programmers to document standards functions results in redundant, useless documentation. I have seen enough comments like "Destructor destructs" "Copy constructor copies" over the years to question the rule.

Please note also that this does not only affect constructors. It is possible to delete other functions too, and this sometimes used to prevent type promotions, e.g. when I want to prevent users from accidentally calling a method f(bool) with a pointer argument.

Concerning your PS, it was suggested to move the A(A const&) = delete into the private declaration block. This would suppress the violation, and the behavior would still be the same because it is still deleted. Nobody can access the deleted function. However, this is not idiomatic.

Cheers,
Jens

[lcintrat]

Ok, your explanation is clear ! :)

I will have a look on the rule this weekend I think.

Regarding the PS, I had in mind without the =delete.