RESOLVED FIXED209581
MESSAGE_CHECK base macros should use UNLIKELY() 
https://bugs.webkit.org/show_bug.cgi?id=209581
Summary MESSAGE_CHECK base macros should use UNLIKELY()
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
DetailsFormatted DiffDiff
Patch for landing (2.56 KB, patch)
2020-03-26 08:46 PDT, David Kilzer (:ddkilzer) 		no flags
DetailsFormatted DiffDiff
Patch for landing (2.49 KB, patch)
2020-03-26 09:02 PDT, David Kilzer (:ddkilzer) 		no flags
DetailsFormatted DiffDiff
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.