[webkit-reviews] review granted: [Bug 206460] [bmalloc] Make use of LockHolder strict in some methods of Scavenger : [Attachment 388119] PATCH

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 206460: [bmalloc] Make use of LockHolder strict in some methods of
Scavenger
https://bugs.webkit.org/show_bug.cgi?id=206460

Attachment 388119: PATCH

https://bugs.webkit.org/attachment.cgi?id=388119&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 388119
  --> https://bugs.webkit.org/attachment.cgi?id=388119
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.