[webkit-reviews] review granted: [Bug 169794] WebAssembly: spec-tests/memory.wast.js fails in debug : [Attachment 305246] patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted JF Bastien
<jfbastien at apple.com>'s request for review:
Bug 169794: WebAssembly: spec-tests/memory.wast.js fails in debug
https://bugs.webkit.org/show_bug.cgi?id=169794

Attachment 305246: patch

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




--- Comment #14 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 305246
  --> https://bugs.webkit.org/attachment.cgi?id=305246
patch

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

r=me if you fix the copyright thingy.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:171
> +    , m_mode(MemoryMode::BoundsChecking)

Nit: I don't think you need this. It has an initializer in the class
definition.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:235
> +	   // FIXME: should this ever occur?
https://bugs.webkit.org/show_bug.cgi?id=169890

I think we have proven this can happen. I'm not sure this FIXME is needed.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:298
> +	       // FIXME: should this ever occur?
https://bugs.webkit.org/show_bug.cgi?id=169890

ditto.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:65
> +	   // because the page protection detect out-of-bounds accesses.

typo: detects

> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:2
> + * Copyright (C) 2016-2017 Apple Inc. All rights reserved.

Did we have changes in here earlier that we forgot to update? If so, I think
you should state that's why you are changing the copyright in the Changelog.

> Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:2
> + * Copyright (C) 2016-2017 Apple Inc. All rights reserved.

ditto.