Bug 46589

Summary: Add WTF_MAKE_NONCOPYABLE macro
Product: WebKit Reporter: Anders Carlsson <andersca>
Component: Web Template FrameworkAssignee: Anders Carlsson <andersca>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, darin, webkit.review.bot, yong.li.webkit, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 42998, 48505, 49897    
Attachments:
Description Flags
Patch
none
Patch ap: review+

Description Anders Carlsson 2010-09-26 15:54:22 PDT
Add WTF_MAKE_NONCOPYABLE macro
Comment 1 Anders Carlsson 2010-09-26 15:59:25 PDT
Created attachment 68864 [details]
Patch
Comment 2 WebKit Review Bot 2010-09-26 16:05:25 PDT
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.
Comment 3 WebKit Review Bot 2010-09-26 17:33:24 PDT
Attachment 68864 [details] did not build on win:
Build output: http://queues.webkit.org/results/4087096
Comment 4 Adam Barth 2010-09-26 21:02:42 PDT
Comment on attachment 68864 [details]
Patch

ok
Comment 5 Alexey Proskuryakov 2010-09-27 10:54:45 PDT
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.
Comment 6 Anders Carlsson 2010-09-27 11:15:41 PDT
(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.
Comment 7 Anders Carlsson 2010-09-27 11:20:47 PDT
Created attachment 68934 [details]
Patch
Comment 8 WebKit Review Bot 2010-09-27 11:22:58 PDT
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.
Comment 9 Anders Carlsson 2010-09-27 12:19:32 PDT
Committed r68414: <http://trac.webkit.org/changeset/68414>
Comment 10 Zoltan Horvath 2010-10-19 02:24:29 PDT
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.
Comment 11 Anders Carlsson 2010-10-27 10:28:59 PDT
(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.
Comment 12 Zoltan Horvath 2010-10-28 02:37:12 PDT
(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?
Comment 13 Darin Adler 2010-10-28 12:23:08 PDT
(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.