Bug 209581

Summary:

MESSAGE_CHECK base macros should use UNLIKELY()

Product:

WebKit

Reporter:

David Kilzer (:ddkilzer) <ddkilzer>

Component:

WebKit2

Assignee:

David Kilzer (:ddkilzer) <ddkilzer>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, useafterfree, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch v1	none
Patch for landing	none
Patch for landing	none

David Kilzer (:ddkilzer)

Reported 2020-03-25 19:52:38 PDT

MESSAGE_CHECK base macros should use UNLIKELY().

Attachments
Patch v1 (2.30 KB, patch) 2020-03-25 20:01 PDT, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch for landing (2.56 KB, patch) 2020-03-26 08:46 PDT, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch for landing (2.49 KB, patch) 2020-03-26 09:02 PDT, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2020-03-25 19:52:56 PDT

<rdar://problem/60901307>

David Kilzer (:ddkilzer)

Comment 2 2020-03-25 20:01:12 PDT

Created attachment 394572 [details] Patch v1

youenn fablet

Comment 3 2020-03-26 05:26:08 PDT

Comment on attachment 394572 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=394572&action=review > Source/WebKit/Platform/IPC/Connection.h:83 > ASSERT(assertion); \ Seems a bit odd to test assertion twice.

youenn fablet

Comment 4 2020-03-26 05:26:36 PDT

Maybe we should put the ASSERT() before the if check. Or add ASSERT_NOT_REACHED() instead.

David Kilzer (:ddkilzer)

Comment 5 2020-03-26 08:16:12 PDT

(In reply to youenn fablet from comment #4) > Maybe we should put the ASSERT() before the if check. > Or add ASSERT_NOT_REACHED() instead. I had the same concern when I first saw the macros (especially if the expression didn't evaluate the same way twice), but I ended up realizing the benefit was that the UI Process would crash on debug builds with the exact assertion text. Maybe we could use ASSERT_NOT_REACHED_WITH_MESSAGE() and use the C-preprocessor trick of changing `assertion` into a literal text string?

David Kilzer (:ddkilzer)

Comment 6 2020-03-26 08:46:15 PDT

Created attachment 394615 [details] Patch for landing

David Kilzer (:ddkilzer)

Comment 7 2020-03-26 08:54:43 PDT

(In reply to David Kilzer (:ddkilzer) from comment #5) > (In reply to youenn fablet from comment #4) > > Maybe we should put the ASSERT() before the if check. > > Or add ASSERT_NOT_REACHED() instead. > > I had the same concern when I first saw the macros (especially if the > expression didn't evaluate the same way twice), but I ended up realizing the > benefit was that the UI Process would crash on debug builds with the exact > assertion text. > > Maybe we could use ASSERT_NOT_REACHED_WITH_MESSAGE() and use the > C-preprocessor trick of changing `assertion` into a literal text string? Actually, putting the ASSERT() before the `if` check is more idiomatic. I'll do that.

David Kilzer (:ddkilzer)

Comment 8 2020-03-26 09:02:26 PDT

Created attachment 394617 [details] Patch for landing

EWS

Comment 9 2020-03-26 09:26:11 PDT

Committed r259047: <https://trac.webkit.org/changeset/259047> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394617 [details].

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