Bug 46589 - Add WTF_MAKE_NONCOPYABLE macro
Summary: Add WTF_MAKE_NONCOPYABLE macro
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Anders Carlsson
URL:
Keywords:
Depends on:
Blocks: 42998 48505 49897
  Show dependency treegraph
 
Reported: 2010-09-26 15:54 PDT by Anders Carlsson
Modified: 2010-11-22 01:47 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.51 KB, patch)
2010-09-26 15:59 PDT, Anders Carlsson
no flags Details | Formatted Diff | Diff
Patch (4.02 KB, patch)
2010-09-27 11:20 PDT, Anders Carlsson
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.