[webkit-reviews] review granted: [Bug 181256] Enable -Wcast-qual for WebInspectorUI, WebKitLegacy, WebKit projects : [Attachment 330656] Patch v4

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 181256: Enable -Wcast-qual for WebInspectorUI, WebKitLegacy, WebKit
projects
https://bugs.webkit.org/show_bug.cgi?id=181256

Attachment 330656: Patch v4

https://bugs.webkit.org/attachment.cgi?id=330656&action=review




--- Comment #12 from Darin Adler <darin at apple.com> ---
Comment on attachment 330656
  --> https://bugs.webkit.org/attachment.cgi?id=330656
Patch v4

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

Probably can land this exactly as is. I have a few suggestions for
improvements, but I suppose they could come later.

> Source/WebKit/Platform/cocoa/WKCrashReporter.mm:42
> +    if (char* oldMessage = const_cast<char*>(CRGetCrashLogMessage()))
> +	   free(oldMessage);

Please do return and fix the bug where we free the old message even if it’s not
the one we set.

> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:197
>      case SecCertificate:
> -	   encode(encoder, (SecCertificateRef)typeRef);
> +	   encode(encoder,
static_cast<SecCertificateRef>(const_cast<void*>(typeRef)));
>	   return;
>      case SecIdentity:
> -	   encode(encoder, (SecIdentityRef)(typeRef));
> +	   encode(encoder,
static_cast<SecIdentityRef>(const_cast<void*>(typeRef)));
>	   return;
>  #if HAVE(SEC_KEYCHAIN)
>      case SecKeychainItem:
> -	   encode(encoder, (SecKeychainItemRef)typeRef);
> +	   encode(encoder,
static_cast<SecKeychainItemRef>(const_cast<void*>(typeRef)));
>	   return;
>  #endif
>  #if HAVE(SEC_ACCESS_CONTROL)
>      case SecAccessControl:
> -	   encode(encoder, (SecAccessControlRef)typeRef);
> +	   encode(encoder,
static_cast<SecAccessControlRef>(const_cast<void*>(typeRef)));
>	   return;
>  #endif
>  #if HAVE(SEC_TRUST_SERIALIZATION)
>      case SecTrust:
> -	   encode(encoder, (SecTrustRef)typeRef);
> +	   encode(encoder,
static_cast<SecTrustRef>(const_cast<void*>(typeRef)));
>	   return;
>  #endif

Why not use checked_cf_cast for these instead?

> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:404
>	   userInfo =
adoptCF(CFDictionaryCreateMutableCopy(kCFAllocatorDefault,
CFDictionaryGetCount(userInfo.get()) + 1, userInfo.get()));
> -	   CFDictionarySetValue((CFMutableDictionaryRef)userInfo.get(),
NSUnderlyingErrorKey, underlyingNSError.get());
> +	  
CFDictionarySetValue(const_cast<CFMutableDictionaryRef>(userInfo.get()),
NSUnderlyingErrorKey, underlyingNSError.get());

This should be done without the cast, like this:

    auto mutableUserInfo =
adoptCF(CFDictionaryCreateMutableCopy(kCFAllocatorDefault,
CFDictionaryGetCount(userInfo.get()) + 1, userInfo.get()));
    CFDictionarySetValue(mutableUserInfo.get(), NSUnderlyingErrorKey,
underlyingNSError.get());
    userInfo = WTFMove(mutableUserInfo);

> Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:2
> + * Copyright (C) 2005-2018 Apple Inc.  All rights reserved.

Patch is showing this file as a removed and then added rather than moved and
modified, so:

1) I think we are losing the history on this file unnecessarily
2) I was not able to spot the changes so did not review them

> Source/WebKitLegacy/mac/WebView/WebPDFView.mm:141
> +    NSString *appPath = [(NSURL *)appURL path];

Some day we should change this to use -[NSURL fileSystemRepresentation] instead
of -[NSURL path].