[webkit-reviews] review granted: [Bug 129181] Auto generate bytecode information for bytecode parser and LLInt : [Attachment 225203] Updated to fix EFL and Windows builds

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 129181: Auto generate bytecode information for bytecode parser and LLInt
https://bugs.webkit.org/show_bug.cgi?id=129181

Attachment 225203: Updated to fix EFL and Windows builds
https://bugs.webkit.org/attachment.cgi?id=225203&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225203&action=review


Looks good.  r=me with issues addressed.  Please also fix the EFL build.  Looks
like it’s failing to generate Bytecodes.h.  Or send email to webkit-dev to
notify EFL engineers of a need for some attention here.

> Source/JavaScriptCore/CMakeLists.txt:614
> +    # changed the command will always be called because the mtime of .h
files will

“.h files” => “the .h files”.

> Source/JavaScriptCore/CMakeLists.txt:616
> +    # Additionally, setting the OBJECT_DEPENDS property will make .h files a
Makefile

“.h files” ==> “the .h files”.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:43
> +		65788A9D18B409EB00C189FF /* Offline Assembler */ = {
> +			isa = PBXAggregateTarget;
> +			buildConfigurationList = 65788AA218B409EB00C189FF /*
Build configuration list for PBXAggregateTarget "Offline Assembler" */;
> +			buildPhases = (
> +				65788AA018B409EB00C189FF /* Generate Derived
Sources */,
> +			);
> +			dependencies = (
> +				65788A9E18B409EB00C189FF /* PBXTargetDependency
*/,
> +			);
> +			name = "Offline Assembler";
> +			productName = "Derived Sources";
> +		};

Is this needed?  I’m no Xcode expert, but this blob suggests that it is for
generating the offline assembler.  If so, how is it that we didn’t need this
before?

Also, it looks suspicious to me that the buildPhases comment says "/* Generate
Derived Sources */“ and the productName says "Derived Sources”.  Are these cut
and paste errors?

> Source/JavaScriptCore/bytecode/BytecodeList.json:61
> +	       { "name" : "op_get_by_id", "length" : 9	},
> +	       { "name" : "op_get_by_id_out_of_line", "length" : 9  },
> +	       { "name" : "op_get_array_length", "length" : 9 },

The original definitions of these bytecodes in Opcode.h has a comment /* has
value profiling */ that is now lost.  If it’s not too much effort, would it be
possible to add support for trailing // comments so that we can preserve those
comments?  If it’s too much effort, we can leave adding comment support for a
later time, and just file a bug to track it so that we don’t forget.

> Source/JavaScriptCore/bytecode/BytecodeList.json:112
> +	       { "name" : "op_get_from_scope", "length" : 8 },

Ditto with the /* has value profiling */ comment.

> Source/JavaScriptCore/bytecode/Opcode.h:65
> +#if ENABLE(LLINT) && ENABLE(LLINT_C_LOOP)
> +    + NUMBER_OF_BYTECODE_HELPER_IDS + NUMBER_OF_CLOOP_BYTECODE_HELPER_IDS
> +#endif

ENABLE(LLINT_C_LOOP) implies ENABLE(LLINT_C_LOOP).  You don’t need to specify
ENABLE(LLINT) here.