[webkit-reviews] review granted: [Bug 172260] [DOMJIT] Move DOMJIT patchpoint infrastructure out of domjit : [Attachment 311421] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted  review:
Bug 172260: [DOMJIT] Move DOMJIT patchpoint infrastructure out of domjit
https://bugs.webkit.org/show_bug.cgi?id=172260

Attachment 311421: Patch

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




--- Comment #15 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 311421
  --> https://bugs.webkit.org/attachment.cgi?id=311421
Patch

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

lgtm.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10270
> +		   FTLSnippetParams domJITParams(*state, params, node, nullptr,
WTFMove(regs), WTFMove(gpScratch), WTFMove(fpScratch));

I wonder if FTLLowerDFGToB3 could just include FTLSnippetParams, which means
you could just say SnippetParams here, and you'd get the FTL one.  We do this
game with FTL::JITCode (which inherits from JSC::JITCode).  I think it's mostly
not confusing, but that's just me.  I like being terse.  But maybe it's
confusing to others.  I don't feel strongly about this.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10367
> +		   Vector<SnippetParams::Value> regs;

If I'm right, you'd still say SnippetParams here - you'd be referring to
FTL::SnippetParams, but that publicly extends JSC::SnippetParams, so this just
works.

> Source/JavaScriptCore/ftl/FTLSnippetParams.h:38
> +class FTLSnippetParams : public SnippetParams {

What about "class SnippetParams : public JSC::SnippetParams"?

This would mirror how JITCode works, for example.  I would only want us to do
this if it doesn't cause us to have to say JSC::SnippetParams everywhere.  OTOH
I think it's OK to say FTL::SnippetParams instead of FTLSnippetParams.