[webkit-reviews] review granted: [Bug 190756] TemplateObject passed to template literal tags are not always identical for the same source location. : [Attachment 369074] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 190756: TemplateObject passed to template literal tags are not always
identical for the same source location.
https://bugs.webkit.org/show_bug.cgi?id=190756

Attachment 369074: Patch

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




--- Comment #25 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 369074
  --> https://bugs.webkit.org/attachment.cgi?id=369074
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:16
> +	   But the top-level one is not created by CodeBlock. This top-level
ScriptExecutable is well-aligned to the Script itself. The top-level
ScriptExecutable has now HashMap,

has now => now has

> Source/JavaScriptCore/runtime/FunctionExecutable.h:339
> +    // FIXME: We can merge rareData pointer and top-level executable
pointer. First time, setting parent. If RareData is required, materialize
RareData, swap it, and store
> +    // top-level executable pointer inside RareData.

If you think we should do this, please file a bug and link it here. Otherwise,
if you don't think it's important, let's remove this FIXME

> Source/JavaScriptCore/runtime/ModuleProgramExecutable.cpp:96
> +auto ModuleProgramExecutable::ensureTemplateObjectMap(VM&) ->
TemplateObjectMap&
> +{
> +    if (m_templateObjectMap)
> +	   return *m_templateObjectMap;
> +    auto result = std::make_unique<TemplateObjectMap>();
> +    WTF::storeStoreFence();
> +    m_templateObjectMap = WTFMove(result);
> +    return *m_templateObjectMap;
> +}

You have this more or less implemented four times. Maybe you can do:

static TemplateObjectMap&
ensureTemplateObjectMapImpl(std::unique_ptr<TemplateObjectMap>& dst)
{
    if (dst)
	return *dst;
    auto result = std::make_unique<TemplateObjectMap>();
    WTF::storeStoreFence();
    dst = WTFMove(result);
    return *dst;
}

And then call this as appropriate from four places.

> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:463
> +    RETURN_IF_EXCEPTION(scope, nullptr);

I think you have a bug if this exception is thrown. Because the next time this
is called, you'll end up returning a null array. I think you might want the
above "!result.isNewEntry" to also check the value in the hashmap. Would be
good to add a test if it's possible.

> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:480
> +	   ASSERT(type() == ModuleProgramExecutableType);
> +	   return
static_cast<ModuleProgramExecutable*>(this)->ensureTemplateObjectMap(vm);

nit: this styling feels weird. Why not just ASSERT_NOT_REACHED() in "default"
case and move this logic up to ModuleProgramExecutableType?