Bug 46589 – Add WTF_MAKE_NONCOPYABLE macro

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Anders Carlsson]

Add WTF_MAKE_NONCOPYABLE macro

[Anders Carlsson]

Created attachment 68864 [details]
Patch

[WebKit Review Bot]

Attachment 68864 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/JSCell.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 68864 [details] did not build on win:
Build output: http://queues.webkit.org/results/4087096

[Adam Barth]

Comment on attachment 68864 [details]
Patch

ok

[Alexey Proskuryakov]

Comment on attachment 68864 [details]
Patch

Not that I'm a fan of CustomAllocated, but isn't this patch breaking it?

r-, because of incredibly poor ChangeLog that doesn't explain why this change is being made.

[Anders Carlsson]

(In reply to comment #5)
> (From update of attachment 68864 [details])
> Not that I'm a fan of CustomAllocated, but isn't this patch breaking it?
>

Not sure what you mean here.
 
> r-, because of incredibly poor ChangeLog that doesn't explain why this change is being made.

Good point, will fix.

[Anders Carlsson]

Created attachment 68934 [details]
Patch

[WebKit Review Bot]

Attachment 68934 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/JSCell.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Committed r68414: <http://trac.webkit.org/changeset/68414>

[Zoltan Horvath]

Unfortunately, we can't use - in many cases - this macro instead of inheriting from Noncopyable, because Noncopyable base class contains protected constructor and destructor but the macro doesn't. Furthermore, we can't put them explicitly inside the macro, because it'll cause compile errors in some cases.

[Anders Carlsson]

(In reply to comment #10)
> Unfortunately, we can't use - in many cases - this macro instead of inheriting from Noncopyable, because Noncopyable base class contains protected constructor and destructor but the macro doesn't. Furthermore, we can't put them explicitly inside the macro, because it'll cause compile errors in some cases.

Could you give an example of this? The whole idea of WTF_MAKE_NONCOPYABLE (And the Noncopyable class) is to completely disallow copying.

[Zoltan Horvath]

(In reply to comment #11)
> Could you give an example of this? The whole idea of WTF_MAKE_NONCOPYABLE (And the Noncopyable class) is to completely disallow copying.

Yes, I know. The problem comes when you want to instantiate a struct or class which hasn't got ctor/dtor since the macro defines copy/assigm ctors but simple ctors, then gcc won't generate default to them. So in cases like this gcc will give compile errors.

Now, I can show only a platform specific case:
-----------------------------------------------------------------------------
diff --git a/WebCore/platform/graphics/qt/FontCustomPlatformData.h b/WebCore/platform/graphics/qt/FontCustomPlatformData.h
index 6c41d47..8a35ceb 100644
--- a/WebCore/platform/graphics/qt/FontCustomPlatformData.h
+++ b/WebCore/platform/graphics/qt/FontCustomPlatformData.h
@@ -32,7 +32,9 @@ namespace WebCore {
 class FontPlatformData;
 class SharedBuffer;
 
-struct FontCustomPlatformData : Noncopyable {
+struct FontCustomPlatformData {
+    WTF_MAKE_NONCOPYABLE(FontCustomPlatformData);
+    public:
     ~FontCustomPlatformData();
 
     // for use with QFontDatabase::addApplicationFont/removeApplicationFont
-----------------------------------------------------------------------------

FontCustomPlatformData hasn't got ctor and gcc don't generate for it, but
FontCustomPlatformData is instantiated by new in WebCore/platform/graphics/qt/FontCustomPlatformDataQt.cpp:59.

Accordingly the compile error:

error: no matching function for call to WebCore::FontCustomPlatformData::FontCustomPlatformData()

I don't know exactly how many cases like this are in WebKit. We can solve the problem with putting an empty ctor to this classes. What do you think, is it the right way?

[Darin Adler]

(In reply to comment #12)
> I don't know exactly how many cases like this are in WebKit. We can solve the problem with putting an empty ctor to this classes. What do you think, is it the right way?

This is a design mistake. We don’t want a class that is noncopyable and then transformed into one that’s copyable.

Lets not use the macro *or* the base class for these cases. Later we can fix them too.