| 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: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
David Kilzer (:ddkilzer)
2020-03-25 19:52:38 PDT
Created attachment 394572 [details]
Patch v1
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. Maybe we should put the ASSERT() before the if check. Or add ASSERT_NOT_REACHED() instead. (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? Created attachment 394615 [details]
Patch for landing
(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. Created attachment 394617 [details]
Patch for landing
Committed r259047: <https://trac.webkit.org/changeset/259047> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394617 [details]. |