Bug 181256

Summary:

Enable -Wcast-qual for WebInspectorUI, WebKitLegacy, WebKit projects

Product:

WebKit

Reporter:

David Kilzer (:ddkilzer) <ddkilzer>

Component:

WebKit Misc.

Assignee:

David Kilzer (:ddkilzer) <ddkilzer>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, cgarcia, commit-queue, darin, ews-watchlist, joepeck, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

177895

Bug Blocks:

181369

Attachments:

Description	Flags
Patch v1	none
Patch v2	none
Patch v3	none
Patch v4	darin: review+
Patch v5	none

David Kilzer (:ddkilzer)

Reported 2018-01-03 13:19:31 PST

Enable -Wcast-qual for WebInspectorUI, WebKitLegacy, WebKit projects after Bug 177895 is fixed.

Attachments
Patch v1 (30.27 KB, patch) 2018-01-05 19:46 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch v2 (68.54 KB, patch) 2018-01-06 03:27 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch v3 (70.51 KB, patch) 2018-01-06 20:15 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch v4 (71.38 KB, patch) 2018-01-07 03:46 PST, David Kilzer (:ddkilzer)	darin: review+	Details Formatted Diff Diff
Patch v5 (54.63 KB, patch) 2018-01-07 19:41 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

David Kilzer (:ddkilzer)

Comment 1 2018-01-03 13:20:14 PST

<rdar://problem/36281730>

David Kilzer (:ddkilzer)

Comment 2 2018-01-05 19:46:28 PST

Created attachment 330629 [details] Patch v1

Joseph Pecoraro

Comment 3 2018-01-05 20:32:11 PST

Comment on attachment 330629 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=330629&action=review Looks good so far. Bot catches a bunch in: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.m > Source/WebKit/Platform/cocoa/WKCrashReporter.mm:48 > - char* oldMessage = (char*)CRGetCrashLogMessage(); > + char* oldMessage = const_cast<char*>(CRGetCrashLogMessage()); > if (oldMessage) > free(oldMessage); Not caused by your changed, but this code raises multiple red flags: - How do we know we can free this data, anyone outside of WebKit could have called CRSetCrashLogMessage with their own string. - Why would we free the oldMessage here, but not care to free any potential value in the earlier case we overwrite the value (line 41-42).

David Kilzer (:ddkilzer)

Comment 4 2018-01-06 01:38:03 PST

Comment on attachment 330629 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=330629&action=review >> Source/WebKit/Platform/cocoa/WKCrashReporter.mm:48 >> free(oldMessage); > > Not caused by your changed, but this code raises multiple red flags: > - How do we know we can free this data, anyone outside of WebKit could have called CRSetCrashLogMessage with their own string. > - Why would we free the oldMessage here, but not care to free any potential value in the earlier case we overwrite the value (line 41-42). Yep, you're correct! As currently written this would leak the previous message if a NULL CFStringRef were passed in. The code to free the old message should happen before the (!infoString) check.

David Kilzer (:ddkilzer)

Comment 5 2018-01-06 03:27:35 PST

Created attachment 330636 [details] Patch v2

EWS Watchlist

Comment 6 2018-01-06 03:30:38 PST

Attachment 330636 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:38: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:57: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:70: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:78: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:79: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:81: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:85: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:86: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:88: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:93: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:94: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:96: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:98: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:100: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:110: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:126: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:138: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:140: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:141: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:143: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:145: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:147: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:149: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:160: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:181: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:184: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:184: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:184: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:185: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:185: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:186: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:186: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:187: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:207: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:210: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:211: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:213: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:214: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:216: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:219: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:221: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:228: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:230: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:231: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:233: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:241: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:251: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:253: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:269: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:273: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:276: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:279: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:280: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:282: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:289: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:291: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:293: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:295: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:297: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:307: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:324: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:327: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:331: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:327: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:337: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:333: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:350: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:355: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:360: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:356: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:368: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:374: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:408: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:413: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 74 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

David Kilzer (:ddkilzer)

Comment 7 2018-01-06 03:32:02 PST

(In reply to Build Bot from comment #6) > Attachment 330636 [details] did not pass style-queue: Will clean up the style issues in WebNSDataExtras.mm in a follow-up patch.

David Kilzer (:ddkilzer)

Comment 8 2018-01-06 20:15:24 PST

Created attachment 330653 [details] Patch v3

EWS Watchlist

Comment 9 2018-01-06 20:17:09 PST

Attachment 330653 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:38: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:57: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:70: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:78: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:79: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:81: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:85: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:86: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:88: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:93: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:94: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:96: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:98: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:100: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:110: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:126: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:138: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:140: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:141: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:143: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:145: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:147: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:149: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:160: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:181: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:184: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:184: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:184: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:185: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:185: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:186: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:186: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:187: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:207: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:210: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:211: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:213: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:214: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:216: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:219: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:221: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:228: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:230: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:231: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:233: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:241: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:251: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:253: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:269: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:273: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:276: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:279: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:280: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:282: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:289: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:291: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:293: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:295: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:297: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:307: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:324: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:327: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:331: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:327: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:337: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:333: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:350: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:355: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:360: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:356: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:368: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:374: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:408: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:413: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 74 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

David Kilzer (:ddkilzer)

Comment 10 2018-01-07 03:46:38 PST

Created attachment 330656 [details] Patch v4

EWS Watchlist

Comment 11 2018-01-07 03:48:17 PST

Attachment 330656 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:38: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:57: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:70: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:78: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:79: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:81: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:85: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:86: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:88: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:93: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:94: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:96: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:98: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:100: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:110: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:126: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:138: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:140: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:141: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:143: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:145: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:147: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:149: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:160: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:181: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:184: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:184: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:184: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:185: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:185: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:186: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:186: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:187: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:207: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:210: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:211: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:213: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:214: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:216: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:219: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:221: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:228: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:230: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:231: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:233: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:241: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:251: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:253: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:269: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:273: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:276: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:279: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:280: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:282: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:289: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:291: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:293: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:295: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:297: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:307: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:324: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:327: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:331: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:327: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:337: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:333: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:350: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:355: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:360: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:356: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:368: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:374: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:408: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:413: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 74 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

Darin Adler

Comment 12 2018-01-07 16:02:32 PST

Comment on attachment 330656 [details] 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].

David Kilzer (:ddkilzer)

Comment 13 2018-01-07 19:32:47 PST

Comment on attachment 330656 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=330656&action=review >> Source/WebKit/Platform/cocoa/WKCrashReporter.mm:42 >> + free(oldMessage); > > Please do return and fix the bug where we free the old message even if it’s not the one we set. This is the bug fix to free the old message. The bug happened if a message had been set previously, then nullptr was passed to clear it out (prior to this change). >> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:197 >> #endif > > Why not use checked_cf_cast for these instead? Because this code above already does a type check to turn the object into an enum: CFType type = typeFromCFTypeRef(typeRef); It seemed redundant to do yet another type check. >> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:404 >> + 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); Will fix. >> 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 1) We are not going to lose history. The option to make git-diff output show the changes with the rename didn't work when I used webkit-patch. 2) I will post another patch and make sure it has the changes. >> 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]. Interestingly, we don't use [NSURL getFileSystemRepresentation:maxLength:] anywhere in WebKit, but we do use [[NSURL path] stringByStandardizingPath] in one place (HTMLConverter.mm) and -stringByStandardizingPath a number of places. Also, WebCore::URL::fileSystemPath() creates a CFURLRef and then uses CFURLCopyFileSystemPath() to get the path. Is there any preferred way of doing this?

David Kilzer (:ddkilzer)

Comment 14 2018-01-07 19:41:03 PST

Created attachment 330667 [details] Patch v5

EWS Watchlist

Comment 15 2018-01-07 19:42:40 PST

Attachment 330667 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:38: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:57: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:70: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:78: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:79: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:81: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:85: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:86: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:88: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:93: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:94: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:96: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:98: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:100: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:110: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:126: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:138: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:140: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:141: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:143: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:145: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:147: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:149: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:160: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:181: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:184: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:184: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:184: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:185: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:185: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:186: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:186: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:187: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:207: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:210: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:211: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:213: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:214: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:216: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:219: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:221: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:228: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:230: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:231: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:233: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:241: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:251: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:253: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:269: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:273: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:276: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:279: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:280: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:282: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:289: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:291: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:293: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:295: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:297: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:307: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:324: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:327: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:331: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:327: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:337: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:333: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:350: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:355: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:360: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:356: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:368: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:374: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:408: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:413: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 74 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

Darin Adler

Comment 16 2018-01-07 21:38:14 PST

Comment on attachment 330656 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=330656&action=review >>> Source/WebKit/Platform/cocoa/WKCrashReporter.mm:42 >>> + free(oldMessage); >> >> Please do return and fix the bug where we free the old message even if it’s not the one we set. > > This is the bug fix to free the old message. > > The bug happened if a message had been set previously, then nullptr was passed to clear it out (prior to this change). Yes, you fixed that and it is not the bug I am referring to The bug I am referring to, something you discussed earlier in this report, I believe is that this code assumes that CRGetCrashLogMessage will return a message string set by this function or null string. But someone else could call CRSetCrashLogMessage. This function needs to instead keep around the string it allocates in a global variable and destroy the old string based on the value of that global, not call CRGetCrashLogMessage. That’s a separate fix that should be done at some point so this is a less dangerous function to use. It’s straightforward to fix; simply add a static char* that holds the value. We’ll know we have it right because we won’t be calling CRGetCrashLogMessage at all and we also need to make sure that we call CRSetCrashLogMessage before destroying the old value. >>> 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]. > > Interestingly, we don't use [NSURL getFileSystemRepresentation:maxLength:] anywhere in WebKit, but we do use [[NSURL path] stringByStandardizingPath] in one place (HTMLConverter.mm) and -stringByStandardizingPath a number of places. > > Also, WebCore::URL::fileSystemPath() creates a CFURLRef and then uses CFURLCopyFileSystemPath() to get the path. > > Is there any preferred way of doing this? Thinking it through I realize I was wrong. It’s fine to use -[NSURL path] here. The main difference between the two methods is that -[NSURL fileSystemRepresentation] converts to a C-style string to make it easy to pass to POSIX functions like fopen, but that is not something we need here; we are using NSWorkspace, which takes an NSString, not a C-style string. The stringByStandardizingPath method is a separate matter, not closely related, and there is almost never a need to use it when using a path to operate on a file. It can be helpful in code that has a reason to store a path for later use, such as in preferences, but exactly what methods to use in that case can be a complex topic.

WebKit Commit Bot

Comment 17 2018-01-07 22:00:46 PST

Comment on attachment 330667 [details] Patch v5 Clearing flags on attachment: 330667 Committed r226499: <https://trac.webkit.org/changeset/226499>

WebKit Commit Bot

Comment 18 2018-01-07 22:00:50 PST

All reviewed patches have been landed. Closing bug.

David Kilzer (:ddkilzer)

Comment 19 2018-01-08 01:54:29 PST

(In reply to WebKit Commit Bot from comment #17) > Comment on attachment 330667 [details] > Patch v5 > > Clearing flags on attachment: 330667 > > Committed r226499: <https://trac.webkit.org/changeset/226499> Build fixes: Committed r226501: <https://trac.webkit.org/changeset/226501> Committed r226503: <https://trac.webkit.org/changeset/226503>

David Kilzer (:ddkilzer)

Comment 20 2018-01-08 02:10:43 PST

(In reply to Darin Adler from comment #16) > Comment on attachment 330656 [details] > Patch v4 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330656&action=review > > >>> Source/WebKit/Platform/cocoa/WKCrashReporter.mm:42 > >>> + free(oldMessage); > >> > >> Please do return and fix the bug where we free the old message even if it’s not the one we set. > > > > This is the bug fix to free the old message. > > > > The bug happened if a message had been set previously, then nullptr was passed to clear it out (prior to this change). > > Yes, you fixed that and it is not the bug I am referring to > > The bug I am referring to, something you discussed earlier in this report, I > believe is that this code assumes that CRGetCrashLogMessage will return a > message string set by this function or null string. But someone else could > call CRSetCrashLogMessage. This function needs to instead keep around the > string it allocates in a global variable and destroy the old string based > on the value of that global, not call CRGetCrashLogMessage. That’s a > separate fix that should be done at some point so this is a less dangerous > function to use. It’s straightforward to fix; simply add a static char* that > holds the value. We’ll know we have it right because we won’t be calling > CRGetCrashLogMessage at all and we also need to make sure that we call > CRSetCrashLogMessage before destroying the old value. The setCrashReportApplicationSpecificInformation() function is already using the gCRAnnotations global defined in WKCrashReporter.mm: // Avoid having to link with libCrashReporterClient.a CRASH_REPORTER_CLIENT_HIDDEN struct crashreporter_annotations_t gCRAnnotations __attribute__((section("__DATA," CRASHREPORTER_ANNOTATIONS_SECTION))) = { CRASHREPORTER_ANNOTATIONS_VERSION, 0, 0, 0, 0, 0, 0, 0 }; <https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/Platform/cocoa/WKCrashReporter.mm#L31> That's how the CRGetCrashLogMessage/CRSetCrashLogMessage macros work: <https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/Platform/spi/Cocoa/CrashReporterClientSPI.h> Sorry if that wasn't clear in my ChangeLog message.

David Kilzer (:ddkilzer)

Comment 21 2018-01-08 02:27:25 PST

(In reply to David Kilzer (:ddkilzer) from comment #20) > (In reply to Darin Adler from comment #16) > > Comment on attachment 330656 [details] > > Patch v4 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=330656&action=review > > > > >>> Source/WebKit/Platform/cocoa/WKCrashReporter.mm:42 > > >>> + free(oldMessage); > > >> > > >> Please do return and fix the bug where we free the old message even if it’s not the one we set. > > > > > > This is the bug fix to free the old message. > > > > > > The bug happened if a message had been set previously, then nullptr was passed to clear it out (prior to this change). > > > > Yes, you fixed that and it is not the bug I am referring to > > > > The bug I am referring to, something you discussed earlier in this report, I > > believe is that this code assumes that CRGetCrashLogMessage will return a > > message string set by this function or null string. But someone else could > > call CRSetCrashLogMessage. This function needs to instead keep around the > > string it allocates in a global variable and destroy the old string based > > on the value of that global, not call CRGetCrashLogMessage. That’s a > > separate fix that should be done at some point so this is a less dangerous > > function to use. It’s straightforward to fix; simply add a static char* that > > holds the value. We’ll know we have it right because we won’t be calling > > CRGetCrashLogMessage at all and we also need to make sure that we call > > CRSetCrashLogMessage before destroying the old value. > > The setCrashReportApplicationSpecificInformation() function is already using > the gCRAnnotations global defined in WKCrashReporter.mm: > > // Avoid having to link with libCrashReporterClient.a > CRASH_REPORTER_CLIENT_HIDDEN > struct crashreporter_annotations_t gCRAnnotations > __attribute__((section("__DATA," CRASHREPORTER_ANNOTATIONS_SECTION))) > = { CRASHREPORTER_ANNOTATIONS_VERSION, 0, 0, 0, 0, 0, 0, 0 }; > > <https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/Platform/cocoa/ > WKCrashReporter.mm#L31> > > That's how the CRGetCrashLogMessage/CRSetCrashLogMessage macros work: > > <https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/Platform/spi/ > Cocoa/CrashReporterClientSPI.h> > > Sorry if that wasn't clear in my ChangeLog message. I guess gCRAnnotations could technically be modified by other code since it's not declared as static, but someone would have to go out of their way to do that since gCRAnnotations is not declared in Source/WebKit/Platform/cocoa/WKCrashReporter.h. Maybe I'm still missing something? Or do we want to be extra careful here by saving a second copy of the char* pointer inside setCrashReportApplicationSpecificInformation()?

Note You need to log in before you can comment on or make changes to this bug.