Bug 217855 – Fix -Wdeprecated-copy warnings in WTF and JavaScriptCore

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Kilzer (:ddkilzer)]

Fix -Wdeprecated-copy warnings in WTF and JavaScriptCore.

1. In JavaScriptCore:

Source/JavaScriptCore/assembler/ARM64Assembler.h:371:14: error: definition of implicit copy constructor for 'LinkRecord' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
Source/JavaScriptCore/assembler/ARM64Assembler.h:371:14: error: definition of implicit copy constructor for 'LinkRecord' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]

2. In WTF (via WebKit):

In file included from WebKitBuild/Release/DerivedSources/WebKit2/unified-sources/UnifiedSource55-mm.mm:3:
In file included from Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:27:
In file included from Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:41:
WebKitBuild/Release/usr/local/include/wtf/Identified.h:42:5: warning: definition of implicit copy assignment operator for 'IdentifiedBase<unsigned long long, WebKit::PDFPlugin::ByteRangeRequest>' is deprecated because it has a user-declared copy constructor [-Wdeprecated-copy]
    IdentifiedBase(const IdentifiedBase& other)
    ^
WebKitBuild/Release/usr/local/include/wtf/Identified.h:57:7: note: in implicit copy assignment operator for 'WTF::IdentifiedBase<unsigned long long, WebKit::PDFPlugin::ByteRangeRequest>' first required here
class Identified : public IdentifiedBase<uint64_t, T> {
      ^
In file included from WebKitBuild/Release/DerivedSources/WebKit2/unified-sources/UnifiedSource55-mm.mm:3:
In file included from Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:27:
Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:381:11: note: in implicit copy assignment operator for 'WTF::Identified<WebKit::PDFPlugin::ByteRangeRequest>' first required here
    class ByteRangeRequest : public Identified<ByteRangeRequest> {
          ^
In file included from WebKitBuild/Release/DerivedSources/WebKit2/unified-sources/UnifiedSource55-mm.mm:1:
In file included from Source/WebKit/WebProcess/Plugins/Netscape/mac/NetscapePluginMac.mm:27:
In file included from Source/WebKit/WebProcess/Plugins/Netscape/NetscapePlugin.h:31:
In file included from Source/WebKit/WebProcess/Plugins/Plugin.h:30:
In file included from WebKitBuild/Release/WebCore.framework/PrivateHeaders/ScrollTypes.h:28:
In file included from WebKitBuild/Release/WebCore.framework/PrivateHeaders/IntPoint.h:28:
In file included from WebKitBuild/Release/WebCore.framework/PrivateHeaders/IntSize.h:29:
In file included from WebKitBuild/Release/usr/local/include/wtf/JSONValues.h:35:
WebKitBuild/Release/usr/local/include/wtf/HashMap.h:351:32: note: in implicit move assignment operator for 'WebKit::PDFPlugin::ByteRangeRequest' first required here
        result.iterator->value = std::forward<V>(value);
                               ^
WebKitBuild/Release/usr/local/include/wtf/HashMap.h:374:12: note: in instantiation of function template specialization 'WTF::HashMap<unsigned long long, WebKit::PDFPlugin::ByteRangeRequest, WTF::DefaultHash<unsigned long long>, WTF::HashTraits<unsigned long long>, WTF::HashTraits<WebKit::PDFPlugin::ByteRangeRequest>>::inlineSet<const unsigned long long &, WebKit::PDFPlugin::ByteRangeRequest>' requested here
    return inlineSet(key, std::forward<T>(mapped));
           ^
In file included from WebKitBuild/Release/DerivedSources/WebKit2/unified-sources/UnifiedSource55-mm.mm:3:
Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:966:36: note: in instantiation of function template specialization 'WTF::HashMap<unsigned long long, WebKit::PDFPlugin::ByteRangeRequest, WTF::DefaultHash<unsigned long long>, WTF::HashTraits<unsigned long long>, WTF::HashTraits<WebKit::PDFPlugin::ByteRangeRequest>>::set<WebKit::PDFPlugin::ByteRangeRequest>' requested here
    m_outstandingByteRangeRequests.set(identifier, WTFMove(request));
                                   ^

<rdar://problem/67716914>

[David Kilzer (:ddkilzer)]

Created attachment 411628 [details]
Patch v1

[Darin Adler]

Comment on attachment 411628 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=411628&action=review

> Source/JavaScriptCore/assembler/ARM64Assembler.h:375
> +            data.copyTypes.content[0] = other.data.copyTypes.content[0];
> +            data.copyTypes.content[1] = other.data.copyTypes.content[1];
> +            data.copyTypes.content[2] = other.data.copyTypes.content[2];

Could we do this with construction syntax? Maybe we can’t because of the union.

> Source/JavaScriptCore/assembler/ARM64Assembler.h:377
> +        ~LinkRecord() = default;

Why add this?

> Source/WTF/wtf/Identified.h:42
> +    IdentifiedBase(const IdentifiedBase&) = default;

This change continues to let the compiler generate a public copy assignment operator, despite the fact that we got out of the way to make the copy constructor protected. It’s more logical that they are either both public or both protected. I suggest we either make both protected by adding the assignment operator explicitly, or both public, by deleting this explicit copy constructor.

[David Kilzer (:ddkilzer)]

Comment on attachment 411628 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=411628&action=review

>> Source/JavaScriptCore/assembler/ARM64Assembler.h:375
>> +            data.copyTypes.content[2] = other.data.copyTypes.content[2];
> 
> Could we do this with construction syntax? Maybe we can’t because of the union.

Yes!, this works:

            : m_data.copyTypes(other.copyTypes)

>> Source/JavaScriptCore/assembler/ARM64Assembler.h:377
>> +        ~LinkRecord() = default;
> 
> Why add this?

Will remove.  Was thinking about "the rule of three" and being explicit about it, but this is redundant.

>> Source/WTF/wtf/Identified.h:42
>> +    IdentifiedBase(const IdentifiedBase&) = default;
> 
> This change continues to let the compiler generate a public copy assignment operator, despite the fact that we got out of the way to make the copy constructor protected. It’s more logical that they are either both public or both protected. I suggest we either make both protected by adding the assignment operator explicitly, or both public, by deleting this explicit copy constructor.

Will fix.

[David Kilzer (:ddkilzer)]

Created attachment 411676 [details]
Patch v2

[Darin Adler]

Comment on attachment 411676 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=411676&action=review

r=me once it compiles

> Source/JavaScriptCore/assembler/ARM64Assembler.h:372
> +            : data.copyTypes(other.copyTypes)

Wow, did not know this syntax existed.

Think this needs to be other.data.copyTypes.

> Source/JavaScriptCore/assembler/ARM64Assembler.h:377
> +            data.copyTypes = other.copyTypes;

Ditto.

> Source/WTF/wtf/Identified.h:56
> +    IdentifiedBase& operator=(const IdentifiedBase& other)
> +    {
> +        m_identifier = other.m_identifier;
> +        return *this;
> +    }

This could also be:

    IdentifiedBase& operator=(const IdentifiedBase&) = default;

And as you did in a previous patch, could use default for the copy constructor above, too. Not sure what your priorities are here. This way also seems fine.

[David Kilzer (:ddkilzer)]

Comment on attachment 411676 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=411676&action=review

>> Source/JavaScriptCore/assembler/ARM64Assembler.h:372
>> +            : data.copyTypes(other.copyTypes)
> 
> Wow, did not know this syntax existed.
> 
> Think this needs to be other.data.copyTypes.

Sigh.  It doesn't.

>> Source/JavaScriptCore/assembler/ARM64Assembler.h:377
>> +            data.copyTypes = other.copyTypes;
> 
> Ditto.

Fixing.

>> Source/WTF/wtf/Identified.h:56
>> +    }
> 
> This could also be:
> 
>     IdentifiedBase& operator=(const IdentifiedBase&) = default;
> 
> And as you did in a previous patch, could use default for the copy constructor above, too. Not sure what your priorities are here. This way also seems fine.

Ah, I see.  Will use = default for both.

[Darin Adler]

Comment on attachment 411676 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=411676&action=review

>>> Source/JavaScriptCore/assembler/ARM64Assembler.h:372
>>> +            : data.copyTypes(other.copyTypes)
>> 
>> Wow, did not know this syntax existed.
>> 
>> Think this needs to be other.data.copyTypes.
> 
> Sigh.  It doesn't.

I meant the "data.copyTypes" syntax; that’s what I didn’t realize existed!

I know the "other.copyTypes" was just a mistake.

[David Kilzer (:ddkilzer)]

Comment on attachment 411676 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=411676&action=review

>>>> Source/JavaScriptCore/assembler/ARM64Assembler.h:372
>>>> +            : data.copyTypes(other.copyTypes)
>>> 
>>> Wow, did not know this syntax existed.
>>> 
>>> Think this needs to be other.data.copyTypes.
>> 
>> Sigh.  It doesn't.
> 
> I meant the "data.copyTypes" syntax; that’s what I didn’t realize existed!
> 
> I know the "other.copyTypes" was just a mistake.

The "data.copyTypes" syntax in the initializer list does not work.

>>> Source/JavaScriptCore/assembler/ARM64Assembler.h:377
>>> +            data.copyTypes = other.copyTypes;
>> 
>> Ditto.
> 
> Fixing.

Interesting:

/Users/ddkilzer/Data/WebKit.git/Source/JavaScriptCore/assembler/ARM64Assembler.h:377:36: error: array type 'uint64_t [3]' is not assignable
            data.copyTypes.content = other.data.copyTypes.content;
            ~~~~~~~~~~~~~~~~~~~~~~ ^

I checked via standalone test case (on x86_54 macOS) and this works:

            data.copyTypes = other.copyTypes;

Ae thee any compatibility issues with using this syntax?

[David Kilzer (:ddkilzer)]

Created attachment 411677 [details]
Patch v3

[David Kilzer (:ddkilzer)]

Comment on attachment 411676 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=411676&action=review

>>>> Source/JavaScriptCore/assembler/ARM64Assembler.h:377
>>>> +            data.copyTypes = other.copyTypes;
>>> 
>>> Ditto.
>> 
>> Fixing.
> 
> Interesting:
> 
> /Users/ddkilzer/Data/WebKit.git/Source/JavaScriptCore/assembler/ARM64Assembler.h:377:36: error: array type 'uint64_t [3]' is not assignable
>             data.copyTypes.content = other.data.copyTypes.content;
>             ~~~~~~~~~~~~~~~~~~~~~~ ^
> 
> I checked via standalone test case (on x86_54 macOS) and this works:
> 
>             data.copyTypes = other.copyTypes;
> 
> Ae thee any compatibility issues with using this syntax?

Still copying and pasting the typo:

            data.copyTypes = other.data.copyTypes;

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #8)
> Are there any compatibility issues with using this syntax?

No, you can assign a struct even if the struct contains an array.

[EWS]

Committed r268657: <https://trac.webkit.org/changeset/268657>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411677 [details].