Bug 206460

Created attachment 388119 [details] PATCH

Comment on attachment 388119 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=388119&action=review Saying review+, but there is something wrong here. > Source/bmalloc/bmalloc/Scavenger.h:57 > - void run(); > + BINLINE void run(); It’s *not* OK to have a public function marked inline that is not defined in the header. This is code that won’t compile in some scenarios, where the code is not part of the appropriate translation unit. Not sure what the situation is. Could be one of these: a) This member function already gets inlined for callers inside Scavenger.cpp even if we don’t mark it with BINLINE. So we don’t need "always inline" nor do we need to mark this function as inline at all, and we should not make this change. b) This member function is never called outside the Scavenger class so it can be made private. But it does need to be marked "always inline", so we should keep this BINLINE but also move it from public to private. c) This member function is called outside the Scavenger class but only inside Scavenger.cpp, so the patch is OK as-is, but we’ll have a problem waiting for us if Ian the future anyone adds any new calls to the function outside Scavenger.cpp. d ...) Some more complicated situation. Perhaps one with no obvious solution? I’m guessing it’s (a) and no change should be made here, but I’m not sure. > Source/bmalloc/bmalloc/Scavenger.h:60 > - void runSoon(); > + BINLINE void runSoon(); Ditto.

Comment on attachment 388119 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=388119&action=review >> Source/bmalloc/bmalloc/Scavenger.h:57 >> + BINLINE void run(); > > It’s *not* OK to have a public function marked inline that is not defined in the header. This is code that won’t compile in some scenarios, where the code is not part of the appropriate translation unit. Not sure what the situation is. Could be one of these: > > a) This member function already gets inlined for callers inside Scavenger.cpp even if we don’t mark it with BINLINE. So we don’t need "always inline" nor do we need to mark this function as inline at all, and we should not make this change. > > b) This member function is never called outside the Scavenger class so it can be made private. But it does need to be marked "always inline", so we should keep this BINLINE but also move it from public to private. > > c) This member function is called outside the Scavenger class but only inside Scavenger.cpp, so the patch is OK as-is, but we’ll have a problem waiting for us if Ian the future anyone adds any new calls to the function outside Scavenger.cpp. > > d ...) Some more complicated situation. Perhaps one with no obvious solution? > > I’m guessing it’s (a) and no change should be made here, but I’m not sure. Ah. Good point. Thanks. As original code doesn't have BINLINE, we should leave this as is, no BINLINE and hope compiler do well for us.

Created attachment 388318 [details] PATCH

Created attachment 388326 [details] PATCH

The commit-queue encountered the following flaky tests while processing attachment 388326 [details]: imported/w3c/web-platform-tests/IndexedDB/fire-error-event-exception.html bug 201481 (authors: shvaikalesh@gmail.com and youennf@gmail.com) The commit-queue is continuing to process your patch.

Comment on attachment 388326 [details] PATCH Clearing flags on attachment: 388326 Committed r254871: <https://trac.webkit.org/changeset/254871>

All reviewed patches have been landed. Closing bug.

<rdar://problem/58769352>