Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1698403002:
[Interpreter] generate-bytecode-expectations improvements. (Closed)
|
Created:
4 years, 10 months ago by Stefano Sanfilippo Modified:
4 years, 10 months ago CC:
v8-reviews_googlegroups.com, oth, rmcilroy Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[Interpreter] generate-bytecode-expectations improvements.
A few options and features have been added to the tool:
* an output file might be specified using --output=file.name
* a shortcut when the output file is also the input, which is handy
when fixing golden files, --rebaseline.
* the input snippet might be optionally not wrapped in a top function,
or not executed after compilation (--no-wrap and --no-execute).
* the name of the wrapper can be configured using --wrapper-name=foo
The same options can be configured via setters on the usual
BytecodeExpectationsPrinter.
The output file now includes all the relevant flags to reproduce it
when running again through the tool (usually with --rebaseline).
In particular, when running in --rebaseline mode, options from the
file header will override options specified in the command line.
A couple of other fixes and improvements:
* description of the handlers is now emitted (closing the TODO).
* the snippet is now correctly unquoted when double quotes are used.
* special registers (closure, context etc.) are now emitted as such,
instead of displaying their numeric value.
* the tool can now process top level code as well.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/d2187182a7ef2183d27c245a99317444d88cd78f
Cr-Commit-Position: refs/heads/master@{#34152}
Patch Set 1 #
Total comments: 37
Patch Set 2 : First round of reviews. #Patch Set 3 : Capital letters where missing. #Patch Set 4 : Change API, always emit banner, clearer I/O handling. #Patch Set 5 : Reorganize help message, --fix => --rebaseline for real. #
Total comments: 18
Patch Set 6 : Remove const qual from methods where we don't need it. #Patch Set 7 : Revert removal of const qualifier. #Patch Set 8 : Second review. #Patch Set 9 : Rename input_file to input_file_handle to make the difference with input_stream more evident. #Patch Set 10 : Reverting to the old API, remove PrepareI/OStream. #
Total comments: 6
Patch Set 11 : Consolidate ProgramOptions, remove unneeded setters. #Patch Set 12 : Re-validate options only upon change. #Patch Set 13 : Ignore runtime errors. #Patch Set 14 : rename wrapper-name, merge kInteger and kDouble, top level code. #
Total comments: 4
Patch Set 15 : Renaming #
Messages
Total messages: 45 (21 generated)
Description was changed from ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --fix. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo * the "generated by" comment banner can be skipped with --no-banner The same options can be configured via getters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --fix). A couple of other fixes and improvements: * now the snippet is correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --fix. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo * the "generated by" comment banner can be skipped with --no-banner The same options can be configured via getters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --fix). A couple of other fixes and improvements: * handlers parameters are now emitted (closing the TODO). * now the snippet is correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ==========
ssanfilippo@chromium.org changed reviewers: + oth@chromium.org, rmcilroy@chromium.org
Description was changed from ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --fix. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo * the "generated by" comment banner can be skipped with --no-banner The same options can be configured via getters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --fix). A couple of other fixes and improvements: * handlers parameters are now emitted (closing the TODO). * now the snippet is correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --fix. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo * the "generated by" comment banner can be skipped with --no-banner The same options can be configured via getters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --fix). A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * now the snippet is correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ==========
I've added a few features to generate-bytecode-expectations and the associated library file that will be useful or necessary for porting the tests. I also took the occasion to close a couple of TODOs and FIXMEs. PTAL.
Looks good, couple of comments. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... File test/cctest/interpreter/bytecode-expectations-printer.cc (right): https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.cc:52: return result; Not using the result? If not, just drop the return value. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.cc:118: stream << '(' << register_value.index() << ')'; Just realized, could you also special case parameter registers (e.g, is_parameter() -> "arg" << register_value.ToParameterIndex(parameter_count) You will need to pass in the function's parameter_count (which is accessable as bytecode_array()->parameter_count() https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.cc:274: if (execute_) Run(script); Out of interest, when do you not want to run the code? https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... File test/cctest/interpreter/bytecode-expectations-printer.h (right): https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.h:47: void set_execute(bool e) { execute_ = e; } don't abbreviate -> /s/e/execute/ Also pleas update set_constant_pool_type (missed this in the previous review) https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.h:50: void set_wrap(bool w) { wrap_ = w; } ditto https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.h:53: void set_top_function_name(const std::string& s) { top_function_name_ = s; } ditto https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:189: const char* BooleanString(bool value) { return value ? "yes" : "no"; } nit - move up near ParseBoolean https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:218: } Could you emit a seperator between the options and the first snippet too (e.g., another "---") and use it to end the loop in ParseOptionsHeader https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:247: You should also validate that --rebaseline and --raw-js aren't specified together. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:304: // If it was not an escape sequence, emit the backslash /s/the backslash/the previous backslash./ https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:327: ParseOptionsHeader(options, body_stream); This should only happen for --rebaseline I think (and should probably be moved out of ExtractSnippetsFromStream and instead called directly from main (i.e., immediately after the are read from the command line). https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:336: ProgramOptions* options) { This should still be const, right? Any reason to move to a pointer rather than reference? https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:392: " --fix Overwrite input file.\n" Call this "--rebaseline Rebaseline input snippet file" https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:393: " --no-banner Do not add the \"generated by\" banner.\n" Why is this one required? All golden files should have this banner shouldn't they?
https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... File test/cctest/interpreter/bytecode-expectations-printer.cc (right): https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.cc:52: return result; On 2016/02/16 16:43:48, rmcilroy wrote: > Not using the result? If not, just drop the return value. Done. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.cc:118: stream << '(' << register_value.index() << ')'; On 2016/02/16 16:43:48, rmcilroy wrote: > Just realized, could you also special case parameter registers (e.g, > is_parameter() -> "arg" << register_value.ToParameterIndex(parameter_count) > > You will need to pass in the function's parameter_count (which is accessable as > bytecode_array()->parameter_count() Done. If I understand correctly, the format you expected is like R(arg1), R(arg2), etc. Right? https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.cc:274: if (execute_) Run(script); On 2016/02/16 16:43:48, rmcilroy wrote: > Out of interest, when do you not want to run the code? Apparently, MakeTopLevelBytecode does not run the code after compiling it (and it does not wrap it either). That behaviour is recreated in our case by setting both wrap_ and execute_ to false. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... File test/cctest/interpreter/bytecode-expectations-printer.h (right): https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.h:47: void set_execute(bool e) { execute_ = e; } On 2016/02/16 16:43:48, rmcilroy wrote: > don't abbreviate -> /s/e/execute/ > Also pleas update set_constant_pool_type (missed this in the previous review) Done. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.h:50: void set_wrap(bool w) { wrap_ = w; } On 2016/02/16 16:43:48, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.h:53: void set_top_function_name(const std::string& s) { top_function_name_ = s; } On 2016/02/16 16:43:48, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:189: const char* BooleanString(bool value) { return value ? "yes" : "no"; } On 2016/02/16 16:43:48, rmcilroy wrote: > nit - move up near ParseBoolean Done. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:218: } On 2016/02/16 16:43:48, rmcilroy wrote: > Could you emit a seperator between the options and the first snippet too (e.g., > another "---") and use it to end the loop in ParseOptionsHeader I have updated the code to use the separator of the next block (namely, the first expectation) to end the parsing of the header. Is it OK? Also, I've made the parser a bit more robust by rejecting any line which is neither a known configuration option nor a blank one. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:247: On 2016/02/16 16:43:48, rmcilroy wrote: > You should also validate that --rebaseline and --raw-js aren't specified > together. Done. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:304: // If it was not an escape sequence, emit the backslash On 2016/02/16 16:43:48, rmcilroy wrote: > /s/the backslash/the previous backslash./ Done. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:327: ParseOptionsHeader(options, body_stream); On 2016/02/16 16:43:48, rmcilroy wrote: > This should only happen for --rebaseline I think (and should probably be moved > out of ExtractSnippetsFromStream and instead called directly from main (i.e., > immediately after the are read from the command line). Please see below. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:336: ProgramOptions* options) { On 2016/02/16 16:43:48, rmcilroy wrote: > This should still be const, right? Any reason to move to a pointer rather than > reference? The reason is that ParseOptionsHeader called inside ExtractSnippetsFromStream can and will modify ProgramOptions. I realise it's not super straightforward, so I'm trying to move the ParseOptionsHeader call as close to the main in the call graph as possible. Unfortunately we can't call it directly in the main, because we need to determine and prepare the input stream (either stdin or file), which is exactly the task performed by ExtractSnippets before calling ExtractSnippetsFromStream. I have moved the incriminated call to ExtraxtSnippets and I have also renamed it to MaybeUpdateOptionsFromHeader in order to make it clearer that program options might (or might not) be modified. I am open to other solutions, though. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:392: " --fix Overwrite input file.\n" On 2016/02/16 16:43:48, rmcilroy wrote: > Call this "--rebaseline Rebaseline input snippet file" Done. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:393: " --no-banner Do not add the \"generated by\" banner.\n" On 2016/02/16 16:43:48, rmcilroy wrote: > Why is this one required? All golden files should have this banner shouldn't > they? ATM the banner is emitted by the utility before calling the library, because PrintExpectation is called with one snippet at a time, so we would end up repeating the header before each snippet if the call was moved there. I am still thinking about the best way of emitting the header while using the library outside the utility. Probably something like PrintHeader with an internal consistency check in PrintExpectation (e.g. CHECK(header_emitted_)). This is a temporary solution, but IMHO it might as well remain as it's not adding complexity to the code.
Description was changed from ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --fix. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo * the "generated by" comment banner can be skipped with --no-banner The same options can be configured via getters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --fix). A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * now the snippet is correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo * the "generated by" comment banner can be skipped with --no-banner The same options can be configured via getters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --fix). A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ==========
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698403002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698403002/40001
Description was changed from ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo * the "generated by" comment banner can be skipped with --no-banner The same options can be configured via getters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --fix). A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo * the "generated by" comment banner can be skipped with --no-banner The same options can be configured via setters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --fix). A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo * the "generated by" comment banner can be skipped with --no-banner The same options can be configured via setters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --fix). A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo * the "generated by" comment banner can be skipped with --no-banner The same options can be configured via setters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --rebaseline). A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... File test/cctest/interpreter/bytecode-expectations-printer.cc (right): https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.cc:118: stream << '(' << register_value.index() << ')'; On 2016/02/16 20:28:26, ssanfilippo wrote: > On 2016/02/16 16:43:48, rmcilroy wrote: > > Just realized, could you also special case parameter registers (e.g, > > is_parameter() -> "arg" << register_value.ToParameterIndex(parameter_count) > > > > You will need to pass in the function's parameter_count (which is accessable > as > > bytecode_array()->parameter_count() > > Done. > > If I understand correctly, the format you expected is like R(arg1), R(arg2), > etc. Right? Yes. Could you also special-case <this> e.g., see Register::ToString. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.cc:274: if (execute_) Run(script); On 2016/02/16 20:28:26, ssanfilippo wrote: > On 2016/02/16 16:43:48, rmcilroy wrote: > > Out of interest, when do you not want to run the code? > > Apparently, MakeTopLevelBytecode does not run the code after compiling it (and > it does not wrap it either). That behaviour is recreated in our case by setting > both wrap_ and execute_ to false. Ahh I see, yeah you are right, top level code will get compiled to bytecode even if not executed. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:336: ProgramOptions* options) { On 2016/02/16 20:28:26, ssanfilippo wrote: > On 2016/02/16 16:43:48, rmcilroy wrote: > > This should still be const, right? Any reason to move to a pointer rather than > > reference? > > The reason is that ParseOptionsHeader called inside ExtractSnippetsFromStream > can and will modify ProgramOptions. I realise it's not super straightforward, so > I'm trying to move the ParseOptionsHeader call as close to the main in the call > graph as possible. > > Unfortunately we can't call it directly in the main, because we need to > determine and prepare the input stream (either stdin or file), which is exactly > the task performed by ExtractSnippets before calling ExtractSnippetsFromStream. > > I have moved the incriminated call to ExtraxtSnippets and I have also renamed it > to MaybeUpdateOptionsFromHeader in order to make it clearer that program options > might (or might not) be modified. > > I am open to other solutions, though. I think the solution is to split out the opening of the file, creation of the stream and then reading of the header as seperate functions from ExtractSnippets - with ExtractSnippets just taking the already open stream, WDYT? https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:336: ProgramOptions* options) { On 2016/02/16 20:28:26, ssanfilippo wrote: > On 2016/02/16 16:43:48, rmcilroy wrote: > > This should still be const, right? Any reason to move to a pointer rather than > > reference? > > The reason is that ParseOptionsHeader called inside ExtractSnippetsFromStream > can and will modify ProgramOptions. I realise it's not super straightforward, so > I'm trying to move the ParseOptionsHeader call as close to the main in the call > graph as possible. > > Unfortunately we can't call it directly in the main, because we need to > determine and prepare the input stream (either stdin or file), which is exactly > the task performed by ExtractSnippets before calling ExtractSnippetsFromStream. > > I have moved the incriminated call to ExtraxtSnippets and I have also renamed it > to MaybeUpdateOptionsFromHeader in order to make it clearer that program options > might (or might not) be modified. > > I am open to other solutions, though. I think the solution is to split out the opening of the file, creation of the stream and then reading of the header as seperate functions from ExtractSnippets - with ExtractSnippets just taking the already open stream, WDYT? https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:393: " --no-banner Do not add the \"generated by\" banner.\n" On 2016/02/16 20:28:26, ssanfilippo wrote: > On 2016/02/16 16:43:48, rmcilroy wrote: > > Why is this one required? All golden files should have this banner shouldn't > > they? > > ATM the banner is emitted by the utility before calling the library, because > PrintExpectation is called with one snippet at a time, so we would end up > repeating the header before each snippet if the call was moved there. I am still > thinking about the best way of emitting the header while using the library > outside the utility. Probably something like PrintHeader with an internal > consistency check in PrintExpectation (e.g. CHECK(header_emitted_)). This is a > temporary solution, but IMHO it might as well remain as it's not adding > complexity to the code. I think the banner should be printed by this tool once at the top of the file (i.e., before the program options), that way you can remove this from the flags passed to BytecodeExpectationPrinter.
https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... File test/cctest/interpreter/bytecode-expectations-printer.cc (right): https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.cc:118: stream << '(' << register_value.index() << ')'; On 2016/02/17 10:16:59, rmcilroy wrote: > On 2016/02/16 20:28:26, ssanfilippo wrote: > > On 2016/02/16 16:43:48, rmcilroy wrote: > > > Just realized, could you also special case parameter registers (e.g, > > > is_parameter() -> "arg" << register_value.ToParameterIndex(parameter_count) > > > > > > You will need to pass in the function's parameter_count (which is accessable > > as > > > bytecode_array()->parameter_count() > > > > Done. > > > > If I understand correctly, the format you expected is like R(arg1), R(arg2), > > etc. Right? > > Yes. Could you also special-case <this> e.g., see Register::ToString. Done. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/byt... test/cctest/interpreter/bytecode-expectations-printer.cc:274: if (execute_) Run(script); On 2016/02/17 10:16:59, rmcilroy wrote: > On 2016/02/16 20:28:26, ssanfilippo wrote: > > On 2016/02/16 16:43:48, rmcilroy wrote: > > > Out of interest, when do you not want to run the code? > > > > Apparently, MakeTopLevelBytecode does not run the code after compiling it (and > > it does not wrap it either). That behaviour is recreated in our case by > setting > > both wrap_ and execute_ to false. > > Ahh I see, yeah you are right, top level code will get compiled to bytecode even > if not executed. Done. https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:336: ProgramOptions* options) { On 2016/02/17 10:16:59, rmcilroy wrote: > On 2016/02/16 20:28:26, ssanfilippo wrote: > > On 2016/02/16 16:43:48, rmcilroy wrote: > > > This should still be const, right? Any reason to move to a pointer rather > than > > > reference? > > > > The reason is that ParseOptionsHeader called inside ExtractSnippetsFromStream > > can and will modify ProgramOptions. I realise it's not super straightforward, > so > > I'm trying to move the ParseOptionsHeader call as close to the main in the > call > > graph as possible. > > > > Unfortunately we can't call it directly in the main, because we need to > > determine and prepare the input stream (either stdin or file), which is > exactly > > the task performed by ExtractSnippets before calling > ExtractSnippetsFromStream. > > > > I have moved the incriminated call to ExtraxtSnippets and I have also renamed > it > > to MaybeUpdateOptionsFromHeader in order to make it clearer that program > options > > might (or might not) be modified. > > > > I am open to other solutions, though. > > I think the solution is to split out the opening of the file, creation of the > stream and then reading of the header as seperate functions from ExtractSnippets > - with ExtractSnippets just taking the already open stream, WDYT? I've rewritten the piece using a better approach which allows us to move MaybeUpdateOptionsFromHeader in the main(). Not as compact as I wish it could be, but still better than before. WDYT? https://codereview.chromium.org/1698403002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:393: " --no-banner Do not add the \"generated by\" banner.\n" On 2016/02/17 10:17:00, rmcilroy wrote: > On 2016/02/16 20:28:26, ssanfilippo wrote: > > On 2016/02/16 16:43:48, rmcilroy wrote: > > > Why is this one required? All golden files should have this banner shouldn't > > > they? > > > > ATM the banner is emitted by the utility before calling the library, because > > PrintExpectation is called with one snippet at a time, so we would end up > > repeating the header before each snippet if the call was moved there. I am > still > > thinking about the best way of emitting the header while using the library > > outside the utility. Probably something like PrintHeader with an internal > > consistency check in PrintExpectation (e.g. CHECK(header_emitted_)). This is a > > temporary solution, but IMHO it might as well remain as it's not adding > > complexity to the code. > > I think the banner should be printed by this tool once at the top of the file > (i.e., before the program options), that way you can remove this from the flags > passed to BytecodeExpectationPrinter. Done. I ended up changing the API from accepting a single snippet per call to taking an std::vector of snippets.
Description was changed from ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo * the "generated by" comment banner can be skipped with --no-banner The same options can be configured via setters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --rebaseline). A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo The same options can be configured via setters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --rebaseline). A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ==========
https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... File test/cctest/interpreter/bytecode-expectations-printer.cc (right): https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/bytecode-expectations-printer.cc:56: (void)result; Just remove result variable and instead call: CHECK(!script->Run(isolate_->GetCurrentContext().IsEmpty()) https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/bytecode-expectations-printer.cc:334: stream << "#\n# Autogenerated by generate-bytecode-expectations\n#\n\n"; I meant to move this into the generate-bytecode-expectations. You should also do the printing of the options in generate-bytecode-expectations (since that is where you do the parsing of those options). https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... File test/cctest/interpreter/bytecode-expectations-printer.h (right): https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/bytecode-expectations-printer.h:65: void PrintOptionsHeader(std::ostream& stream); // NOLINT const https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:66: friend void MaybeUpdateOptionsFromHeader(ProgramOptions*, std::istream&); Don't make this a friend, just add setters for the options you might want to change (and rely on the object's constness for ensuring the object options don't change in ExtractSnippts or elsewhere). https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:376: std::istream& body_stream = options.read_from_stdin() ? std::cin : body_file; Extract lines 366-376 into a function which returns the std::istream (by reference as an output pointer parameter) https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:377: MaybeUpdateOptionsFromHeader(&options, body_stream); I would do: if (options->rebaseline) { UpdateOptionsFromHeader(&options, body_stream); } (removing the Maybe from the function name) https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:395: } Also pull out the ostream creation to a helper function.
https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... File test/cctest/interpreter/bytecode-expectations-printer.cc (right): https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/bytecode-expectations-printer.cc:56: (void)result; On 2016/02/17 15:24:08, rmcilroy wrote: > Just remove result variable and instead call: > > CHECK(!script->Run(isolate_->GetCurrentContext().IsEmpty()) Done. https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/bytecode-expectations-printer.cc:334: stream << "#\n# Autogenerated by generate-bytecode-expectations\n#\n\n"; On 2016/02/17 15:24:08, rmcilroy wrote: > I meant to move this into the generate-bytecode-expectations. You should also do > the printing of the options in generate-bytecode-expectations (since that is > where you do the parsing of those options). The issue is that if we don't have this code inside the library, the tests will not be able to exactly reproduce the same format of generate-bytecode-expecations. https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... File test/cctest/interpreter/bytecode-expectations-printer.h (right): https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/bytecode-expectations-printer.h:65: void PrintOptionsHeader(std::ostream& stream); // NOLINT On 2016/02/17 15:24:08, rmcilroy wrote: > const Done. https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:66: friend void MaybeUpdateOptionsFromHeader(ProgramOptions*, std::istream&); On 2016/02/17 15:24:08, rmcilroy wrote: > Don't make this a friend, just add setters for the options you might want to > change (and rely on the object's constness for ensuring the object options don't > change in ExtractSnippts or elsewhere). Done. https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:376: std::istream& body_stream = options.read_from_stdin() ? std::cin : body_file; On 2016/02/17 15:24:08, rmcilroy wrote: > Extract lines 366-376 into a function which returns the std::istream (by > reference as an output pointer parameter) Done. I had to resort to a less elegant way of doing this. Since it's not straightforward, I have added a comment explaining why I had to implement it that way. https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:377: MaybeUpdateOptionsFromHeader(&options, body_stream); On 2016/02/17 15:24:08, rmcilroy wrote: > I would do: > if (options->rebaseline) { > UpdateOptionsFromHeader(&options, body_stream); > } > > (removing the Maybe from the function name) Done. https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:395: } On 2016/02/17 15:24:08, rmcilroy wrote: > Also pull out the ostream creation to a helper function. Done, see above.
https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... File test/cctest/interpreter/bytecode-expectations-printer.cc (right): https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/bytecode-expectations-printer.cc:334: stream << "#\n# Autogenerated by generate-bytecode-expectations\n#\n\n"; On 2016/02/17 16:40:49, ssanfilippo wrote: > On 2016/02/17 15:24:08, rmcilroy wrote: > > I meant to move this into the generate-bytecode-expectations. You should also > do > > the printing of the options in generate-bytecode-expectations (since that is > > where you do the parsing of those options). > > The issue is that if we don't have this code inside the library, the tests will > not be able to exactly reproduce the same format of > generate-bytecode-expecations. The tests should just ignore the headers and only diff the snippets I think. https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:376: std::istream& body_stream = options.read_from_stdin() ? std::cin : body_file; On 2016/02/17 16:40:50, ssanfilippo wrote: > On 2016/02/17 15:24:08, rmcilroy wrote: > > Extract lines 366-376 into a function which returns the std::istream (by > > reference as an output pointer parameter) > > Done. I had to resort to a less elegant way of doing this. Since it's not > straightforward, I have added a comment explaining why I had to implement it > that way. Sorry, when I said go back to the way you were originally doing I meant doing it inline in main, could you do that please?
https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... File test/cctest/interpreter/bytecode-expectations-printer.cc (right): https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/bytecode-expectations-printer.cc:334: stream << "#\n# Autogenerated by generate-bytecode-expectations\n#\n\n"; On 2016/02/17 17:06:36, rmcilroy wrote: > On 2016/02/17 16:40:49, ssanfilippo wrote: > > On 2016/02/17 15:24:08, rmcilroy wrote: > > > I meant to move this into the generate-bytecode-expectations. You should > also > > do > > > the printing of the options in generate-bytecode-expectations (since that is > > > where you do the parsing of those options). > > > > The issue is that if we don't have this code inside the library, the tests > will > > not be able to exactly reproduce the same format of > > generate-bytecode-expecations. > > The tests should just ignore the headers and only diff the snippets I think. OK, then I'll make the test harness skip the golden file header. https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1698403002/diff/80001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:376: std::istream& body_stream = options.read_from_stdin() ? std::cin : body_file; On 2016/02/17 17:06:36, rmcilroy wrote: > On 2016/02/17 16:40:50, ssanfilippo wrote: > > On 2016/02/17 15:24:08, rmcilroy wrote: > > > Extract lines 366-376 into a function which returns the std::istream (by > > > reference as an output pointer parameter) > > > > Done. I had to resort to a less elegant way of doing this. Since it's not > > straightforward, I have added a comment explaining why I had to implement it > > that way. > > Sorry, when I said go back to the way you were originally doing I meant doing it > inline in main, could you do that please? Done. Sorry for the misunderstanding :)
LGTM once last few comments are addressed. https://codereview.chromium.org/1698403002/diff/170001/test/cctest/interprete... File test/cctest/interpreter/bytecode-expectations-printer.h (right): https://codereview.chromium.org/1698403002/diff/170001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations-printer.h:59: void reset_top_function_name() { Unused. I'd remove this one, seems unnecessary and the name is confusing. https://codereview.chromium.org/1698403002/diff/170001/test/cctest/interprete... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1698403002/diff/170001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:171: void UpdateOptionsFromHeader(ProgramOptions* options, Nit - make this a member function of ProgramOptions (and call UpdateFromHeader()) https://codereview.chromium.org/1698403002/diff/170001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:334: void PrintOptionsHeader(std::ostream& stream, // NOLINT, Also make this a member on ProgramOptions and call just call it Print(). Also move all the helper functions (ConstantPoolTypeToString, BooleanToString, ParseBoolean, ParseConstantPoolType) private member functions on ProgramOptions too (that way you can probably actually get rid of the setters you added previously).
https://codereview.chromium.org/1698403002/diff/170001/test/cctest/interprete... File test/cctest/interpreter/bytecode-expectations-printer.h (right): https://codereview.chromium.org/1698403002/diff/170001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations-printer.h:59: void reset_top_function_name() { On 2016/02/18 09:58:33, rmcilroy wrote: > Unused. I'd remove this one, seems unnecessary and the name is confusing. Done. https://codereview.chromium.org/1698403002/diff/170001/test/cctest/interprete... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1698403002/diff/170001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:171: void UpdateOptionsFromHeader(ProgramOptions* options, On 2016/02/18 09:58:33, rmcilroy wrote: > Nit - make this a member function of ProgramOptions (and call > UpdateFromHeader()) Done. https://codereview.chromium.org/1698403002/diff/170001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:334: void PrintOptionsHeader(std::ostream& stream, // NOLINT, On 2016/02/18 09:58:33, rmcilroy wrote: > Also make this a member on ProgramOptions and call just call it Print(). Also > move all the helper functions (ConstantPoolTypeToString, BooleanToString, > ParseBoolean, ParseConstantPoolType) private member functions on ProgramOptions > too (that way you can probably actually get rid of the setters you added > previously). Done the first part. I have grouped helpers at the beginnning of this cc file, but I'd prefer them to live outside the class, since they are not bound to fields in any way.
Description was changed from ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo The same options can be configured via setters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --rebaseline). A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo The same options can be configured via setters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --rebaseline). In particular, when running in --rebaseline mode, options from the file header will override options specified in the command line. A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ==========
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698403002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698403002/210001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698403002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698403002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo The same options can be configured via setters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --rebaseline). In particular, when running in --rebaseline mode, options from the file header will override options specified in the command line. A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo The same options can be configured via setters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --rebaseline). In particular, when running in --rebaseline mode, options from the file header will override options specified in the command line. A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. * the tool can now process top level code as well. BUG=v8:4280 LOG=N ==========
LGTM with a renaming suggestion. https://codereview.chromium.org/1698403002/diff/250001/test/cctest/interprete... File test/cctest/interpreter/bytecode-expectations-printer.h (right): https://codereview.chromium.org/1698403002/diff/250001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations-printer.h:55: void set_top_level_code(bool top_level_code) { /s/top_level_code/top_level/ (throughout and in generate_bytecode_expectations ProgramOptions too). https://codereview.chromium.org/1698403002/diff/250001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations-printer.h:60: void set_top_function_name(const std::string& top_function_name) { Let's call this test_function_name to avoid confusion with top_level (throughout and in generate_bytecode_expectations ProgramOptions too).
https://codereview.chromium.org/1698403002/diff/250001/test/cctest/interprete... File test/cctest/interpreter/bytecode-expectations-printer.h (right): https://codereview.chromium.org/1698403002/diff/250001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations-printer.h:55: void set_top_level_code(bool top_level_code) { On 2016/02/19 11:50:04, rmcilroy wrote: > /s/top_level_code/top_level/ (throughout > and in generate_bytecode_expectations ProgramOptions too). Done. https://codereview.chromium.org/1698403002/diff/250001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations-printer.h:60: void set_top_function_name(const std::string& top_function_name) { On 2016/02/19 11:50:04, rmcilroy wrote: > Let's call this test_function_name to avoid confusion with top_level (throughout > and in generate_bytecode_expectations ProgramOptions too). Done.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698403002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698403002/270001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssanfilippo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1698403002/#ps270001 (title: "Renaming")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698403002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698403002/270001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo The same options can be configured via setters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --rebaseline). In particular, when running in --rebaseline mode, options from the file header will override options specified in the command line. A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. * the tool can now process top level code as well. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo The same options can be configured via setters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --rebaseline). In particular, when running in --rebaseline mode, options from the file header will override options specified in the command line. A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. * the tool can now process top level code as well. BUG=v8:4280 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #15 (id:270001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo The same options can be configured via setters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --rebaseline). In particular, when running in --rebaseline mode, options from the file header will override options specified in the command line. A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. * the tool can now process top level code as well. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] generate-bytecode-expectations improvements. A few options and features have been added to the tool: * an output file might be specified using --output=file.name * a shortcut when the output file is also the input, which is handy when fixing golden files, --rebaseline. * the input snippet might be optionally not wrapped in a top function, or not executed after compilation (--no-wrap and --no-execute). * the name of the wrapper can be configured using --wrapper-name=foo The same options can be configured via setters on the usual BytecodeExpectationsPrinter. The output file now includes all the relevant flags to reproduce it when running again through the tool (usually with --rebaseline). In particular, when running in --rebaseline mode, options from the file header will override options specified in the command line. A couple of other fixes and improvements: * description of the handlers is now emitted (closing the TODO). * the snippet is now correctly unquoted when double quotes are used. * special registers (closure, context etc.) are now emitted as such, instead of displaying their numeric value. * the tool can now process top level code as well. BUG=v8:4280 LOG=N Committed: https://crrev.com/d2187182a7ef2183d27c245a99317444d88cd78f Cr-Commit-Position: refs/heads/master@{#34152} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/d2187182a7ef2183d27c245a99317444d88cd78f Cr-Commit-Position: refs/heads/master@{#34152} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
