Last round of WebCore fixes for new clang compiler. <rdar://problem/33667497>
Created attachment 323061 [details] Patch v1
Debug EWS is red: /Volumes/Data/EWS/WebKit/Source/WebCore/Modules/cache/WorkerCacheStorageConnection.cpp:136:71: error: variable 'cacheIdentifier' cannot be implicitly captured in a lambda with no capture-default specified ASSERT(!result.hasValue() || result.value().identifier == cacheIdentifier);
Comment on attachment 323061 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=323061&action=review > Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:141 > +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) > +#define _CCRSACryptorCreateFromData_arg_data_cast(value) (const_cast<uint8_t*>(value)) > +#else > +#define _CCRSACryptorCreateFromData_arg_data_cast(value) (value) > +#endif This doesn’t need to be a macro; it can be an inline function, and we could try to give it a more normal WebKit-style name. Why the strange macro name with an underscore? Names that start with an underscore and a capital letter are technically reserved, so it’s not a great idea to use a name like that. > Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:57 > + attributesDictionary = adoptCF(CFDictionaryCreateCopy(nullptr, mutableAttributes.get())); Since this sidesteps the type safety issue by making a copy of the dictionary, we need to be sure it doesn’t make things too slow. On the other hand, performance of these things changes over time anyway. I think there is some new copy-on-write logic that makes this kind of idiom fast with NSDictionary; not sure if the Foundation programmers made it do the same thing when using the CFDictionary functions. Also, above we used kCFAllocatorDefault, but here we are using nullptr for the same thing.
Comment on attachment 323061 [details] Patch v1 Obsoleting since I need to fix the Debug build and address Darin's feedback.
(In reply to Darin Adler from comment #3) > Comment on attachment 323061 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323061&action=review > > > Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:141 > > +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) > > +#define _CCRSACryptorCreateFromData_arg_data_cast(value) (const_cast<uint8_t*>(value)) > > +#else > > +#define _CCRSACryptorCreateFromData_arg_data_cast(value) (value) > > +#endif > > This doesn’t need to be a macro; it can be an inline function, and we could > try to give it a more normal WebKit-style name. > > Why the strange macro name with an underscore? Names that start with an > underscore and a capital letter are technically reserved, so it’s not a > great idea to use a name like that. Changed to an inline C++ function. > > Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:57 > > + attributesDictionary = adoptCF(CFDictionaryCreateCopy(nullptr, mutableAttributes.get())); > > Since this sidesteps the type safety issue by making a copy of the > dictionary, we need to be sure it doesn’t make things too slow. On the other > hand, performance of these things changes over time anyway. I think there is > some new copy-on-write logic that makes this kind of idiom fast with > NSDictionary; not sure if the Foundation programmers made it do the same > thing when using the CFDictionary functions. I'm checking to see if the CoW performance improvement also affects CFDictionary. > Also, above we used kCFAllocatorDefault, but here we are using nullptr for > the same thing. Fixed.
(In reply to David Kilzer (:ddkilzer) from comment #5) > (In reply to Darin Adler from comment #3) > > Comment on attachment 323061 [details] > > Patch v1 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=323061&action=review > > > > > Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:57 > > > + attributesDictionary = adoptCF(CFDictionaryCreateCopy(nullptr, mutableAttributes.get())); > > > > Since this sidesteps the type safety issue by making a copy of the > > dictionary, we need to be sure it doesn’t make things too slow. On the other > > hand, performance of these things changes over time anyway. I think there is > > some new copy-on-write logic that makes this kind of idiom fast with > > NSDictionary; not sure if the Foundation programmers made it do the same > > thing when using the CFDictionary functions. > > I'm checking to see if the CoW performance improvement also affects > CFDictionary. Turns out the CoW performance fix does NOT apply to CFDictionary, but the current code suggests there are at most 3 entries. I could also change this code to use NSMutableDictionary/NSDictionary and change the source file to be Objective-C++, then use the toll-free-bridge to return a CFDictionary object. To be honest, I'm tempted just to revert to the previous behavior of assigning the CFMutableDictionary to the RetainPtr<CFDictionary> and call it good since there were no known issues with that code as written.
(In reply to David Kilzer (:ddkilzer) from comment #6) > (In reply to David Kilzer (:ddkilzer) from comment #5) > > (In reply to Darin Adler from comment #3) > > > Comment on attachment 323061 [details] > > > Patch v1 > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=323061&action=review > > > > > > > Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:57 > > > > + attributesDictionary = adoptCF(CFDictionaryCreateCopy(nullptr, mutableAttributes.get())); > > > > > > Since this sidesteps the type safety issue by making a copy of the > > > dictionary, we need to be sure it doesn’t make things too slow. On the other > > > hand, performance of these things changes over time anyway. I think there is > > > some new copy-on-write logic that makes this kind of idiom fast with > > > NSDictionary; not sure if the Foundation programmers made it do the same > > > thing when using the CFDictionary functions. > > > > I'm checking to see if the CoW performance improvement also affects > > CFDictionary. > > Turns out the CoW performance fix does NOT apply to CFDictionary, but the > current code suggests there are at most 3 entries. > > I could also change this code to use NSMutableDictionary/NSDictionary and > change the source file to be Objective-C++, then use the toll-free-bridge to > return a CFDictionary object. > > To be honest, I'm tempted just to revert to the previous behavior of > assigning the CFMutableDictionary to the RetainPtr<CFDictionary> and call it > good since there were no known issues with that code as written. I backed out this change as it's not needed yet; it will be revisited with: Bug 177895: Re-enable -Wcast-qual for Apple ports
Created attachment 323440 [details] Patch v2
Comment on attachment 323440 [details] Patch v2 /Volumes/Data/EWS/WebKit/Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:45:8: error: unknown type name 'unint8_t' inline unint8_t* castDataArgumentToCCRSACryptorCreateFromDataIfNeeded(const uint8_t* value) ^ /Volumes/Data/EWS/WebKit/Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:47:12: error: cannot initialize return object of type 'int *' with an rvalue of type 'uint8_t *' (aka 'unsigned char *') return const_cast<uint8_t*>(value); ^~~~~~~~~~~~~~~~~~~~~~~~~~~ /Volumes/Data/EWS/WebKit/Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:149:30: error: no matching function for call to 'CCRSACryptorCreateFromData' CCCryptorStatus status = CCRSACryptorCreateFromData( ^~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:32: /Volumes/Data/EWS/WebKit/Source/WebCore/crypto/CommonCryptoUtilities.h:129:28: note: candidate function not viable: no known conversion from 'int *' to 'const uint8_t *' (aka 'const unsigned char *') for 2nd argument extern "C" CCCryptorStatus CCRSACryptorCreateFromData(CCRSAKeyType keyType, const uint8_t *modulus, size_t modulusLength, const uint8_t *exponent, size_t exponentLength, const uint8_t *p, size_t pLength, const uint8_t *q, size_t qLength, CCRSACryptorRef *ref); ^ 3 errors generated.
Created attachment 323446 [details] Patch v3
Created attachment 323473 [details] Patch v4
Comment on attachment 323446 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=323446&action=review > Source/WebCore/ChangeLog:15 > + * Modules/entriesapi/DOMFileSystem.cpp: > + (WebCore::DOMFileSystem::getFile): Remove unused lambda capture > + for 'this'. Chris Dumez checked in a fix for this in r223204. > Source/WebCore/ChangeLog:17 > + * Modules/entriesapi/FileSystemDirectoryEntry.cpp: > + (WebCore::FileSystemDirectoryEntry::getEntry): Ditto. Chris Dumez checked in a fix for this in r223205.
Comment on attachment 323473 [details] Patch v4 r=me
Comment on attachment 323473 [details] Patch v4 Clearing flags on attachment: 323473 Committed r223212: <https://trac.webkit.org/changeset/223212>
All reviewed patches have been landed. Closing bug.