[webkit-reviews] review granted: [Bug 181927] [YARR] Add diagnosis for YarrJIT failures : [Attachment 331906] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Yusuke Suzuki <utatane.tea at gmail.com>'s
request for review:
Bug 181927: [YARR] Add diagnosis for YarrJIT failures
https://bugs.webkit.org/show_bug.cgi?id=181927

Attachment 331906: Patch

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




--- Comment #3 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 331906
  --> https://bugs.webkit.org/attachment.cgi?id=331906
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331906&action=review

> Source/JavaScriptCore/yarr/YarrJIT.cpp:3505
> +    std::optional<JITFailure> m_shouldFallBack;

Perhaps this should be called m_failureReason

> Source/JavaScriptCore/yarr/YarrJIT.h:53
> +enum class JITFailure {

Perhaps a better name is JITFailureReason?

> Source/JavaScriptCore/yarr/YarrJIT.h:85
> +    void setFallBack(std::optional<JITFailure> fallBack) { m_fallBack =
fallBack; }

Perhaps a better name here would be setFallBackWithFailureReason (or something
like that), since you failure reason enum is not what you are falling back to,
but why you are falling back.  I also think this could take a JITFailure
(without the optional), since you never seem to call this with nullopt.