Why does clang-tidy recommend deleted member functions should be public?

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Why does clang-tidy recommend deleted member functions should be public?

Eric Fiselier via cfe-dev
The modernize-use-equals-delete check despite not saying much about it in the docs thinks that deleted member functions should be public (https://github.com/llvm-mirror/clang-tools-extra/blob/master/test/clang-tidy/modernize-use-equals-delete.cpp#L145) and it's thus going off on Qt's Q_DISABLE_COPY (http://doc.qt.io/qt-5/qobject.html#Q_DISABLE_COPY) macro.

The guidelines from Qt are that this macro be private, not public, and it's unclear to us why clang-tidy thinks so. Could anyone shed some light on the reasoning?

Thanks,
Vadim

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Why does clang-tidy recommend deleted member functions should be public?

Eric Fiselier via cfe-dev
There's nothing inherently wrong with deleting member functions that are private, but if they are public, you will get a better error message (namely that the function is deleted) instead of an error message that says that the functions are private.

AFAIK, Qt supports C++11, but also previous versions. I don't know how they implemented Q_DISABLE_COPY, but it probably expands to copy constructor and copy assignment declarations before C++11, and if you enable C++11 mode, deletes them instead. In pre-C++11, the declarations would have to be private, so you get an error at compile time and not at link time.

If you're only compiling with C++11, it should be safe to put the macro in public. Alternately, you can always delete the copy constructor and assignment yourself.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Why does clang-tidy recommend deleted member functions should be public?

Eric Fiselier via cfe-dev
Thank you for the prompt clarification, Nicolas!

On Tue, Oct 31, 2017 at 7:34 AM Nicolas Lesser <[hidden email]> wrote:
There's nothing inherently wrong with deleting member functions that are private, but if they are public, you will get a better error message (namely that the function is deleted) instead of an error message that says that the functions are private.

AFAIK, Qt supports C++11, but also previous versions. I don't know how they implemented Q_DISABLE_COPY, but it probably expands to copy constructor and copy assignment declarations before C++11, and if you enable C++11 mode, deletes them instead. In pre-C++11, the declarations would have to be private, so you get an error at compile time and not at link time.

If you're only compiling with C++11, it should be safe to put the macro in public. Alternately, you can always delete the copy constructor and assignment yourself.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Why does clang-tidy recommend deleted member functions should be public?

Eric Fiselier via cfe-dev
In reply to this post by Eric Fiselier via cfe-dev
On Mon, Oct 30, 2017 at 11:34 PM, Nicolas Lesser via cfe-dev <[hidden email]> wrote:
There's nothing inherently wrong with deleting member functions that are private, but if they are public, you will get a better error message (namely that the function is deleted) instead of an error message that says that the functions are private.

AFAIK, Qt supports C++11, but also previous versions. I don't know how they implemented Q_DISABLE_COPY, but it probably expands to copy constructor and copy assignment declarations before C++11, and if you enable C++11 mode, deletes them instead. In pre-C++11, the declarations would have to be private, so you get an error at compile time and not at link time.

If you're only compiling with C++11, it should be safe to put the macro in public. Alternately, you can always delete the copy constructor and assignment yourself.

Another reason to prefer making these "public" is non-technical: they affect fundamental aspects of the client interface of a class, and hiding them in "private" as if they were implementation details is unhelpful.  Unfortunately necessary in C++98, but no longer needed with C++11.

-- James


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Why does clang-tidy recommend deleted member functions should be public?

Eric Fiselier via cfe-dev

Long before ‘deleted’ was added to the C++ language, reserving implicitly defined members as ‘private’ members was a very common pattern.  Indeed, I can find many examples where I (and others) routinely used this pattern.  For example, in from code I wrote in 1994 I have the following example (which I still use today -- if it ain’t broke don’t fix it):

 

class MemoryPool {

...

private: // Interface:  remove copying and address-of from the interface

  MemoryPool ( const MemoryPool& );

   MemoryPool& operator = ( const MemoryPool& );

   MemoryPool* operator & ();

...

};

 

Without this pattern, these methods were implicitly defined whether you liked it or not; but with the consistent use of this pattern if these methods were referenced by a non-member there was a compile-time error (access constraint), and if a member accidentally referenced them there was a link-time error (undefined symbol), ‘deleted’ just elevated this to being also a compile-time error.

 

So aside from necessity in modern C++, I would expect that this pattern still persists, and that ‘deleted’ has been added (probably as a macro whose definition is dependent on C++ Standard version for portability)  to such declarations to bring it up to date with the Standard, but they are almost certainly more likely to be ‘private’.  This would make ‘private/deleted’ more likely than ‘public/deleted’, especially in portable legacy code-bases.  The incredibly helpful ‘overloaded’ is often used in a similar way with a Standard dependent macro definition.

 

            MartinO

 

From: cfe-dev [mailto:[hidden email]] On Behalf Of James Dennett via cfe-dev
Sent: 31 October 2017 15:12
To: Nicolas Lesser <[hidden email]>
Cc: Clang Dev <[hidden email]>
Subject: Re: [cfe-dev] Why does clang-tidy recommend deleted member functions should be public?

 

On Mon, Oct 30, 2017 at 11:34 PM, Nicolas Lesser via cfe-dev <[hidden email]> wrote:

There's nothing inherently wrong with deleting member functions that are private, but if they are public, you will get a better error message (namely that the function is deleted) instead of an error message that says that the functions are private.

 

AFAIK, Qt supports C++11, but also previous versions. I don't know how they implemented Q_DISABLE_COPY, but it probably expands to copy constructor and copy assignment declarations before C++11, and if you enable C++11 mode, deletes them instead. In pre-C++11, the declarations would have to be private, so you get an error at compile time and not at link time.

 

If you're only compiling with C++11, it should be safe to put the macro in public. Alternately, you can always delete the copy constructor and assignment yourself.

 

Another reason to prefer making these "public" is non-technical: they affect fundamental aspects of the client interface of a class, and hiding them in "private" as if they were implementation details is unhelpful.  Unfortunately necessary in C++98, but no longer needed with C++11.

 

-- James

 


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev