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>
Created attachment 411628 [details] Patch v1
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.
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.
Created attachment 411676 [details] Patch v2
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.
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.
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.
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?
Created attachment 411677 [details] Patch v3
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;
(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.
Committed r268657: <https://trac.webkit.org/changeset/268657> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411677 [details].