[webkit-reviews] review granted: [Bug 214379] Support export namespace `export * as ns` : [Attachment 408990] Patch

[bugzilla-daemon at webkit.org]

Ross Kirsling <ross.kirsling at sony.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 214379: Support export namespace `export * as ns`
https://bugs.webkit.org/show_bug.cgi?id=214379

Attachment 408990: Patch

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




--- Comment #4 from Ross Kirsling <ross.kirsling at sony.com> ---
Comment on attachment 408990
  --> https://bugs.webkit.org/attachment.cgi?id=408990
Patch

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

r=me. Thanks for the extra explanation over Slack.

> Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:89
> +	       // export { * as v } from "mod"

This form can't use braces though, right?

> Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp:779
> +    // Materialize *namespace* slot with module namespace object.
> +    // If module environment is not yet materialized, we will materialize it
when materializing module environment.

The "it" here is a bit confusing upon first read. Maybe something like
"...unless the module environment is not yet materialized, in which case we'll
do it in setModuleEnvironment"?

> Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp:782
> +	   bool putResult = false;
> +	   symbolTablePutTouchWatchpointSet(m_moduleEnvironment.get(),
globalObject, vm.propertyNames->starNamespacePrivateName,
moduleNamespaceObject, /* shouldThrowReadOnlyError */ false, /*
ignoreReadOnlyErrors */ true, putResult);

nit: I know this is copied code, but it seems silly that one boolean is
described using an identifier and the other two are described with block
comments.