Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1671863002:
[Interpreter] Initial generate-bytecode-expectations implementation. (Closed)
|
Created:
4 years, 10 months ago by Stefano Sanfilippo Modified:
4 years, 10 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[Interpreter] Initial generate-bytecode-expectations implementation.
generate-bytecode-expectations is a tool intended to work together
with test/cctest/test-bytecode-generator.cc in order to produce a
meaningful diff between testcases and the actual bytecode being emitted.
It does so by parsing and compiling Javascript to bytecode,
constructing the same data structure in the testcase and then running a
textual diff between the expected (i.e. the one encoded in the unit test)
and actual (i.e. the one built from the compiler output) representation.
This commit is a first step in this direction, achieving just the first
half of what we desire. At the moment, bytecodechecker can:
* take a code snippet from the command line and emit the expected structure.
* adhere to the same formatting rules of the test cases
(this one is important for text diff and for copy and pasting too)
Still to do:
* parse unit tests:
+ extract code snippets
+ indent the code to match the input test case
+ allow flexibility in the input format
+ try to recognize and work around some macro magic (i.e. REPEAT_127)
* emit the representation of the constant pool and handlers vector
* run a textual diff
BUG=v8:4280
LOG=N
Committed: https://crrev.com/d3604cdb6808789e5f60b3f56f0ec2e992fad56c
Cr-Commit-Position: refs/heads/master@{#33863}
Patch Set 1 #
Total comments: 43
Patch Set 2 : Addressing review comments. #
Total comments: 37
Patch Set 3 : Remove the "static" qualifier from func in the anonymous namespace. #Patch Set 4 : Hopefully fix the Windows declspec issue. #Patch Set 5 : Second round of fixes. #
Total comments: 6
Patch Set 6 : Get rid of RemoveTrailingSpaces, more obvious comment. #Patch Set 7 : Rename tool to generate-bytecode-expectations. #Patch Set 8 : Remove all references to bytecodechecker. #Patch Set 9 : Use std::string as buffer in QuoteCString. #Patch Set 10 : Finally convinced git cl format to respect our style. #
Total comments: 4
Patch Set 11 : Rename V8StringFromUTF8 and function_body #Patch Set 12 : Moved to interpreter/ #
Messages
Total messages: 37 (17 generated)
ssanfilippo@chromium.org changed reviewers: + oth@chromium.org, rmcilroy@chromium.org
Please have a look at this cl.
Looks very good. A few nits, but nothing major. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker.cc File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:13: #include "src/v8.h" Is there something specific from src/v8.h? Samples use include/v8.h and put it above libplatform. Suggest breaking up sections here based on location, e.g. see style guide and existing samples. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:21: void *Allocate(size_t length) override { The predominant style in v8 is "void*" rather than "void *" and ditto reference arguments throughout this file. The Google C++ coding style permits either though. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:35: PlatformWrapper(PlatformWrapper &) = delete; V8 code generally packages this up with (doesn't deal with the move constructor for now, but non-issue at present): DISALLOW_COPY_AND_ASSIGN() The code might also want to: DISALLOW_IMPLICIT_CONSTRUCTORS() https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:201: void PrintBytecodeArray(std::ostream &stream, Something for you and Ross, but consider emitting a comment banner that says how this file is generated and have the diff portion ignore those lines. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:222: if (!emitting_first_instruction) { The code can be simplified here using BytecodeIterator::current_offset() rather than having to toggle a flag. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:238: std::cerr << "Usage: " << argv[0] << " \"return null;\"\n"; This usage message is cryptic. Going forward, it might be better to take a js file name as an argument and/or read from stdin.
Looking good. A couple of more comments. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker.cc File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:46: v8::Isolate *isolate_; use base::SmartPointer<...> for both of these objects (and remove the delete calls in the destructor). https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:54: }; Not used yet - remove for now. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:56: PlatformWrapper::PlatformWrapper(const char *program_name, /s/program_name/exec_path https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:57: const char *wrapper_function_name) { no need to pass this to the PlatformWrapper, just move the setting of i::FLAG_ignition_filter before you compile and run the function itself. In fact, given that we pretty much support all code now, just removing the ignition_filter flag entirely would probably be fine. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:73: i::FLAG_legacy_const = true; just move these to the top of the function, then there shouldn't be any need to call interpreter()->Initialize() (it should happen during V8 initialization if Also, remove the i::FLAG_legacy_const = true; (Mythri has removed this one from test-bytecode-generator in https://codereview.chromium.org/1634153002/). https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:179: stream << "U8(" << op_value_regcount << ')'; This should be U16 https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:181: } Personally I would prefer that this is written using the helpers in the bytecode array iterator. I think you could write it as follows: OperandType op_type = Bytecodes::GetOperandType(bytecode, op_index); OperandSize op_size = Bytecodes::GetOperandSize(bytecode, op_index); const char* size; switch (op_size): case OperandSize::kNone: UNREACHABLE(); break; case OperandSize::kByte: size = "8"; break; case OperandSize::kShort: size = "16"; break; } if (IsRegisterOperandType(op_type) { Register register = bytecode_iter.GetRegisterOperand(op_index); stream << "R" << size << "(" << register.index() << ")"; } else { uint32_t raw_value = bytecode_iter.GetRawOperand(op_index, op_type); stream << "U" << size << "(" << raw_value << ")"; } WDYT? https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:192: stream << "B(" << Bytecodes::ToString(bytecode) << ')'; You will need a comma after the B(..) or the final operand. It's OK to have trailing commas in this sequence so just always emit the comma after B(..) and after every PrintBytecodeOperand should be fine. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:201: void PrintBytecodeArray(std::ostream &stream, On 2016/02/05 11:41:04, oth wrote: > Something for you and Ross, but consider emitting a comment banner that says how > this file is generated and have the diff portion ignore those lines. +1 https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:207: stream << ' ' << frame_size / kPointerSize << " * kPointerSize,\n"; Could you make these indented by the same amount as the test file (7 spaces I think). https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:222: if (!emitting_first_instruction) { On 2016/02/05 11:41:04, oth wrote: > The code can be simplified here using BytecodeIterator::current_offset() rather > than having to toggle a flag. Also, again trailing commas are OK I think, so you could simplify this by just emitting the commas in PrintBytecode. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:238: std::cerr << "Usage: " << argv[0] << " \"return null;\"\n"; On 2016/02/05 11:41:05, oth wrote: > This usage message is cryptic. > > Going forward, it might be better to take a js file name as an argument and/or > read from stdin. > +1, I would have it read from stdin in this CL since future CLs will be reading this from a file anyways. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:245: PlatformWrapper platform(argv[0], wrapper_function_name); I'd rename PlatformWrapper as V8InitializationScope or similar. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:258: std::cout << "{" << '"' << body << "\",\n"; nit - move this into PrintBytecodeArray (and the close brackets below)
Thanks for the feedback, I have amended all the issues you pointed out. PTAL. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker.cc File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:13: #include "src/v8.h" On 2016/02/05 11:41:04, oth wrote: > Is there something specific from src/v8.h? Samples use include/v8.h and put it > above libplatform. No there's nothing specific ATM, I was just following the trend in the other source files, where src/v8.h is used instead of include/... Anyway, I replaced that line with include/v8.h > Suggest breaking up sections here based on location, e.g. see style guide and > existing samples. Done. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:21: void *Allocate(size_t length) override { On 2016/02/05 11:41:05, oth wrote: > The predominant style in v8 is > "void*" rather than "void *" and ditto reference arguments throughout this file. > The Google C++ coding style permits either though. Done. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:35: PlatformWrapper(PlatformWrapper &) = delete; On 2016/02/05 11:41:04, oth wrote: > V8 code generally packages this up with (doesn't deal with the move constructor > for now, but non-issue at present): > > DISALLOW_COPY_AND_ASSIGN() Done. > The code might also want to: > > DISALLOW_IMPLICIT_CONSTRUCTORS() I am not too sure this has any use, since we have no ctor which could be implicit. Should I add it anyway? https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:46: v8::Isolate *isolate_; On 2016/02/05 13:49:51, rmcilroy wrote: > use base::SmartPointer<...> for both of these objects (and remove the delete > calls in the destructor). Done for platform_. base::SmartPointer doesn't seem to handle the Dispose() method Isolate uses for destruction, so I didn't use a smart pointer for that. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:54: }; On 2016/02/05 13:49:51, rmcilroy wrote: > Not used yet - remove for now. Done. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:56: PlatformWrapper::PlatformWrapper(const char *program_name, On 2016/02/05 13:49:51, rmcilroy wrote: > /s/program_name/exec_path Done. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:57: const char *wrapper_function_name) { On 2016/02/05 13:49:51, rmcilroy wrote: > no need to pass this to the PlatformWrapper, just move the setting of > i::FLAG_ignition_filter before you compile and run the function itself. In fact, > given that we pretty much support all code now, just removing the > ignition_filter flag entirely would probably be fine. Ok, for the moment I replaced the filter with a "*" catchall, I'll just drop the line when we remove the flag. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:73: i::FLAG_legacy_const = true; On 2016/02/05 13:49:51, rmcilroy wrote: > just move these to the top of the function Done. > then there shouldn't be any need to > call interpreter()->Initialize() (it should happen during V8 initialization if Actually, not calling Initialize() on the interpreter results in a segfault the first time the Isolate is used. I don't know whether this is the intended behaviour or a bug. > Also, remove the i::FLAG_legacy_const = true; (Mythri has removed this one from > test-bytecode-generator in https://codereview.chromium.org/1634153002/). Done. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:179: stream << "U8(" << op_value_regcount << ')'; On 2016/02/05 13:49:51, rmcilroy wrote: > This should be U16 Silly copy and paste mistake is silly. Fixed. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:181: } On 2016/02/05 13:49:51, rmcilroy wrote: > Personally I would prefer that this is written using the helpers in the bytecode > array iterator. I think you could write it as follows: > > OperandType op_type = Bytecodes::GetOperandType(bytecode, op_index); > OperandSize op_size = Bytecodes::GetOperandSize(bytecode, op_index); > > const char* size; > switch (op_size): > case OperandSize::kNone: > UNREACHABLE(); > break; > case OperandSize::kByte: > size = "8"; > break; > case OperandSize::kShort: > size = "16"; > break; > } > > if (IsRegisterOperandType(op_type) { > Register register = bytecode_iter.GetRegisterOperand(op_index); > stream << "R" << size << "(" << register.index() << ")"; > } else { > uint32_t raw_value = bytecode_iter.GetRawOperand(op_index, op_type); > stream << "U" << size << "(" << raw_value << ")"; > } > > WDYT? Seems OK to me, as long as GetRawOperand() is guaranteed to return a ready-to-print representation of the operand and no further processing is performed by the Get* methods on BytecodeArrayIterator. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:201: void PrintBytecodeArray(std::ostream &stream, On 2016/02/05 11:41:04, oth wrote: > Something for you and Ross, but consider emitting a comment banner that says how > this file is generated and have the diff portion ignore those lines. Done, I have added a boolean flag to print a banner over the generated snippet. Let me know if the message needs to be amended. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:207: stream << ' ' << frame_size / kPointerSize << " * kPointerSize,\n"; On 2016/02/05 13:49:51, rmcilroy wrote: > Could you make these indented by the same amount as the test file (7 spaces I > think). Done. For now the indent amount is hardcoded because it was the easiest option. Let me know if it needs to be configurable. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:222: if (!emitting_first_instruction) { On 2016/02/05 13:49:51, rmcilroy wrote: > On 2016/02/05 11:41:04, oth wrote: > > The code can be simplified here using BytecodeIterator::current_offset() > rather > > than having to toggle a flag. > > Also, again trailing commas are OK I think, so you could simplify this by just > emitting the commas in PrintBytecode. I ended up following oth suggestion, which simplifies the code, but still avoids the trailing comma. I know it's OK, but this way is slightly nicer and it doesn't add a lot of complications (just one extra line). WDYT? https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:238: std::cerr << "Usage: " << argv[0] << " \"return null;\"\n"; On 2016/02/05 13:49:51, rmcilroy wrote: > On 2016/02/05 11:41:05, oth wrote: > > This usage message is cryptic. > > > > Going forward, it might be better to take a js file name as an argument and/or > > read from stdin. > > > > +1, I would have it read from stdin in this CL since future CLs will be reading > this from a file anyways. Done. 0 arguments or "-" means stdin, anything else and we try to open the filename. Also, clarified the help message (now available with --help as well). https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:245: PlatformWrapper platform(argv[0], wrapper_function_name); On 2016/02/05 13:49:51, rmcilroy wrote: > I'd rename PlatformWrapper as V8InitializationScope or similar. Done. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:258: std::cout << "{" << '"' << body << "\",\n"; On 2016/02/05 13:49:51, rmcilroy wrote: > nit - move this into PrintBytecodeArray (and the close brackets below) 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/1671863002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1671863002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
Looks great, a few more comments and suggestions. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker.cc File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:21: void *Allocate(size_t length) override { On 2016/02/08 13:18:34, ssanfilippo wrote: > On 2016/02/05 11:41:05, oth wrote: > > The predominant style in v8 is > > "void*" rather than "void *" and ditto reference arguments throughout this > file. > > The Google C++ coding style permits either though. > > Done. Doesn't look like this has been done. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:181: } On 2016/02/08 13:18:34, ssanfilippo wrote: > On 2016/02/05 13:49:51, rmcilroy wrote: > > Personally I would prefer that this is written using the helpers in the > bytecode > > array iterator. I think you could write it as follows: > > > > OperandType op_type = Bytecodes::GetOperandType(bytecode, op_index); > > OperandSize op_size = Bytecodes::GetOperandSize(bytecode, op_index); > > > > const char* size; > > switch (op_size): > > case OperandSize::kNone: > > UNREACHABLE(); > > break; > > case OperandSize::kByte: > > size = "8"; > > break; > > case OperandSize::kShort: > > size = "16"; > > break; > > } > > > > if (IsRegisterOperandType(op_type) { > > Register register = bytecode_iter.GetRegisterOperand(op_index); > > stream << "R" << size << "(" << register.index() << ")"; > > } else { > > uint32_t raw_value = bytecode_iter.GetRawOperand(op_index, op_type); > > stream << "U" << size << "(" << raw_value << ")"; > > } > > > > WDYT? > > Seems OK to me, as long as GetRawOperand() is guaranteed to return a > ready-to-print representation of the operand and no further processing is > performed by the Get* methods on BytecodeArrayIterator. Yup, I think this should always be safe. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:222: if (!emitting_first_instruction) { On 2016/02/08 13:18:34, ssanfilippo wrote: > On 2016/02/05 13:49:51, rmcilroy wrote: > > On 2016/02/05 11:41:04, oth wrote: > > > The code can be simplified here using BytecodeIterator::current_offset() > > rather > > > than having to toggle a flag. > > > > Also, again trailing commas are OK I think, so you could simplify this by just > > emitting the commas in PrintBytecode. > > I ended up following oth suggestion, which simplifies the code, but still avoids > the trailing comma. I know it's OK, but this way is slightly nicer and it > doesn't add a lot of complications (just one extra line). > > WDYT? SGTM, thanks. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:39: explicit V8InitializationScope(const char *program_name); /s/program_name/exec_path https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:58: i::FLAG_ignition_filter = i::StrDup("*"); just drop this (the default is "*") https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:82: const V8InitializationScope &platform, const char *data) { Just pass in the isolate for these functions (instead of V8InitializationScope) https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:98: v8::Local<v8::Value> CompileAndRun(const V8InitializationScope &platform, ditto https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:106: bool success = script->Run(context).ToLocal(&result); Just: CHECK(script->Run(context).ToLocal(&result)); https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:113: const V8InitializationScope &platform, ditto https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:152: if (!size_tag_is_byte) stream << size_tag; no need for the size_tag_is_byte, just: if (op_size != OperandSize::kByte). Eventually we can just make this consistent and replace the R() macro in test-bytecode-generator with an R8(). https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:173: std::string RemoveTrailingSpaces(const std::string &source) { is this necessary? The tests shouldn't have any trailing spaces in the js snippets, should they (and if so we could just fix them) https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:215: stream << "// ExpectedSnippet generated by bytecodechecker.\n" Add indent before the comment. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:217: "// DO NOT blindly copy and paste into the test suite.\n" Not sure these two lines are necessary (you could put them in PrintUsage if you want though). https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:221: stream << kIndent << "{" << '"' << QuoteCString(RemoveTrailingSpaces(body)) Some comments above each block that you print would be good (e.g., "// Print Javascript snippet.", "Print frame sizes.", etc.) https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:226: if (frame_size > kPointerSize && frame_size % kPointerSize == 0) { frame_size % kPointerSize must always be '0', so no need for that check (you could add a DCHECK below if you want). https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:230: stream << " kPointerSize,\n" << kIndent; No need for this special case, just printing '1 * kPointerSize" is fine https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:234: stream << frame_size << ",\n" << kIndent; given the comment above, this shouldn't be necessary https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:245: // Print separator before each instruction, except the first one full-stop after comments (throughout file). https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:253: stream << kIndent << " // constant pool and handlers here\n"; // placeholder add a // TODO(ssanfillipo): ... above this line instead of the "// placeholder" https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:257: void PrintUsage(const char *exec_path) { nit - PrintUsage just below ReadFromFileOrStdin (above main()) https://codereview.chromium.org/1671863002/diff/20001/test/cctest/cctest.gyp File test/cctest/cctest.gyp (right): https://codereview.chromium.org/1671863002/diff/20001/test/cctest/cctest.gyp#... test/cctest/cctest.gyp:348: 'target_name': 'bytecodechecker', I think we should make the name more specific - e.g., generate-bytecode-expectations?
The CQ bit was checked by oth@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/1671863002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1671863002/20001
The CQ bit was checked by oth@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/1671863002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1671863002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I addressed all the issues, and I still have a couple of comments. PTAL https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker.cc File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:21: void *Allocate(size_t length) override { On 2016/02/08 13:55:31, rmcilroy wrote: > On 2016/02/08 13:18:34, ssanfilippo wrote: > > On 2016/02/05 11:41:05, oth wrote: > > > The predominant style in v8 is > > > "void*" rather than "void *" and ditto reference arguments throughout this > > file. > > > The Google C++ coding style permits either though. > > > > Done. > > Doesn't look like this has been done. It was, but cl format reverted to this style before upload (and I didn't notice). Since both forms are acceptable according to the style guide, we might just leave it as it is. If you prefer, I can do a cl upload without cl format. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:39: explicit V8InitializationScope(const char *program_name); On 2016/02/08 13:55:32, rmcilroy wrote: > /s/program_name/exec_path Done. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:58: i::FLAG_ignition_filter = i::StrDup("*"); On 2016/02/08 13:55:32, rmcilroy wrote: > just drop this (the default is "*") Done. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:82: const V8InitializationScope &platform, const char *data) { On 2016/02/08 13:55:32, rmcilroy wrote: > Just pass in the isolate for these functions (instead of V8InitializationScope) Done. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:98: v8::Local<v8::Value> CompileAndRun(const V8InitializationScope &platform, On 2016/02/08 13:55:32, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:106: bool success = script->Run(context).ToLocal(&result); On 2016/02/08 13:55:31, rmcilroy wrote: > Just: CHECK(script->Run(context).ToLocal(&result)); Done. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:113: const V8InitializationScope &platform, On 2016/02/08 13:55:32, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:152: if (!size_tag_is_byte) stream << size_tag; On 2016/02/08 13:55:32, rmcilroy wrote: > no need for the size_tag_is_byte, just: if (op_size != OperandSize::kByte). Done. > Eventually we can just make this consistent and replace the R() macro in > test-bytecode-generator with an R8(). While this changes goes in the direction of greater uniformity, I can see why R() was chosen in the first place. Both forms are OK for me, and amending the code is very easy. So, whatever you think is best. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:173: std::string RemoveTrailingSpaces(const std::string &source) { On 2016/02/08 13:55:32, rmcilroy wrote: > is this necessary? The tests shouldn't have any trailing spaces in the js > snippets, should they (and if so we could just fix them) This was done to avoid the ".... \n" thing when reading snippet from files, since most text editors add a trailing new line to the file being edited. WDYT? https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:215: stream << "// ExpectedSnippet generated by bytecodechecker.\n" On 2016/02/08 13:55:32, rmcilroy wrote: > Add indent before the comment. Done. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:217: "// DO NOT blindly copy and paste into the test suite.\n" On 2016/02/08 13:55:32, rmcilroy wrote: > Not sure these two lines are necessary (you could put them in PrintUsage if you > want though). Done. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:221: stream << kIndent << "{" << '"' << QuoteCString(RemoveTrailingSpaces(body)) On 2016/02/08 13:55:32, rmcilroy wrote: > Some comments above each block that you print would be good (e.g., "// Print > Javascript snippet.", "Print frame sizes.", etc.) Done. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:226: if (frame_size > kPointerSize && frame_size % kPointerSize == 0) { On 2016/02/08 13:55:32, rmcilroy wrote: > frame_size % kPointerSize must always be '0', so no need for that check (you > could add a DCHECK below if you want). Done. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:230: stream << " kPointerSize,\n" << kIndent; On 2016/02/08 13:55:32, rmcilroy wrote: > No need for this special case, just printing '1 * kPointerSize" is fine This was done for consistency with the test cases in test-bytecode-generator.cc. To tell the truth, both forms are used (cfr. test-bytecode-generator.cc line 141 v. line 542), so again it's the same for me. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:234: stream << frame_size << ",\n" << kIndent; On 2016/02/08 13:55:31, rmcilroy wrote: > given the comment above, this shouldn't be necessary Done. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:245: // Print separator before each instruction, except the first one On 2016/02/08 13:55:32, rmcilroy wrote: > full-stop after comments (throughout file). Done. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:253: stream << kIndent << " // constant pool and handlers here\n"; // placeholder On 2016/02/08 13:55:32, rmcilroy wrote: > add a // TODO(ssanfillipo): ... above this line instead of the "// placeholder" Done. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:257: void PrintUsage(const char *exec_path) { On 2016/02/08 13:55:32, rmcilroy wrote: > nit - PrintUsage just below ReadFromFileOrStdin (above main()) Done.
This is looking pretty good. A couple of optional nits, but fine as is. Leave it Ross and yourself to decide whether to land this now or when progressed further. https://codereview.chromium.org/1671863002/diff/80001/test/cctest/bytecodeche... File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/80001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:148: if (op_size != OperandSize::kByte) stream << size_tag; At some point we should make this consistent, R->R8. https://codereview.chromium.org/1671863002/diff/80001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:178: return trimmed; Nit. This would be slightly more succinct if it didn't copy source, but just found the tail position in source and returned std::string(source.begin(), tail);
Looks great, I'll L-G-T-M once you've renamed the tool (and the cc file and any references to bytecodechecker in the code / output). https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker.cc File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:21: void *Allocate(size_t length) override { On 2016/02/09 10:09:58, ssanfilippo wrote: > On 2016/02/08 13:55:31, rmcilroy wrote: > > On 2016/02/08 13:18:34, ssanfilippo wrote: > > > On 2016/02/05 11:41:05, oth wrote: > > > > The predominant style in v8 is > > > > "void*" rather than "void *" and ditto reference arguments throughout this > > > file. > > > > The Google C++ coding style permits either though. > > > > > > Done. > > > > Doesn't look like this has been done. > > It was, but cl format reverted to this style before upload (and I didn't > notice). Since both forms are acceptable according to the style guide, we might > just leave it as it is. If you prefer, I can do a cl upload without cl format. Hmm that's weird (git cl format doesn't do that for me). Did you replace all instances (including the char* kIndent at the top of the file)? Maybe git cl format takes the first instance it sees and makes the rest of the file consistent. I'd still prefer this to be the same as the rest of the V8 style, but if you can't convince git cl format to play nice then I guess I'll live with it.§ https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/20001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:173: std::string RemoveTrailingSpaces(const std::string &source) { On 2016/02/09 10:09:58, ssanfilippo wrote: > On 2016/02/08 13:55:32, rmcilroy wrote: > > is this necessary? The tests shouldn't have any trailing spaces in the js > > snippets, should they (and if so we could just fix them) > > This was done to avoid the ".... \n" thing when reading snippet from files, > since most text editors add a trailing new line to the file being edited. WDYT? Most editors have an option to remove trailing spaces on save, and since the normal use-case of this tool would be reading from test-bytecode-generator (which already has the trailing spaces trimmed) I'd rather just keep things simple and remove this for now. https://codereview.chromium.org/1671863002/diff/20001/test/cctest/cctest.gyp File test/cctest/cctest.gyp (right): https://codereview.chromium.org/1671863002/diff/20001/test/cctest/cctest.gyp#... test/cctest/cctest.gyp:348: 'target_name': 'bytecodechecker', On 2016/02/08 13:55:32, rmcilroy wrote: > I think we should make the name more specific - e.g., > generate-bytecode-expectations? I'd still like this rename to be done before landing. https://codereview.chromium.org/1671863002/diff/80001/test/cctest/bytecodeche... File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/80001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:211: stream << kIndent << "// ExpectedSnippet generated by bytecodechecker.\n"; nit - add "===" header to make this obvious, i.e., "// === ExpectedSnippet autogenerated by <toolname> ==="
Description was changed from
==========
[Interpreter] Initial bytecodechecker implementation.
bytecodechecker is a tool intended to work together with
test/cctest/test-bytecode-generator.cc in order to produce a meaningful
diff between testcases and the actual bytecode being emitted.
It does so by parsing and compiling Javascript to bytecode,
constructing the same data structure in the testcase and then running a
textual diff between the expected (i.e. the one encoded in the unit test)
and actual (i.e. one built from the compiler output) representation.
This commit is a first step in this direction, achieving just the first
half of what we desire. At the moment, bytecodechecker can:
* take a code snippet from the command line and emit the expected structure.
* adhere to the same formatting rules of the test cases
(this one is important for text diff and for copy and pasting too)
Still to do:
* parse unit tests:
+ extract code snippets
+ indent the code to match the input test case
+ allow flexibility in the input format
+ try to recognize and work around some macro magic (i.e. REPEAT_127)
* emit the representation of the constant pool and handlers vector
* run a textual diff
BUG=v8:4280
LOG=N
==========
to
==========
[Interpreter] Initial bytecodechecker implementation.
generate-bytecode-expectations is a tool intended to work
together with test/cctest/test-bytecode-generator.cc in
order to produce a meaningful diff between testcases and
the actual bytecode being emitted.
It does so by parsing and compiling Javascript to bytecode,
constructing the same data structure in the testcase and then running a
textual diff between the expected (i.e. the one encoded in the unit test)
and actual (i.e. one built from the compiler output) representation.
This commit is a first step in this direction, achieving just the first
half of what we desire. At the moment, bytecodechecker can:
* take a code snippet from the command line and emit the expected structure.
* adhere to the same formatting rules of the test cases
(this one is important for text diff and for copy and pasting too)
Still to do:
* parse unit tests:
+ extract code snippets
+ indent the code to match the input test case
+ allow flexibility in the input format
+ try to recognize and work around some macro magic (i.e. REPEAT_127)
* emit the representation of the constant pool and handlers vector
* run a textual diff
BUG=v8:4280
LOG=N
==========
Description was changed from
==========
[Interpreter] Initial bytecodechecker implementation.
generate-bytecode-expectations is a tool intended to work
together with test/cctest/test-bytecode-generator.cc in
order to produce a meaningful diff between testcases and
the actual bytecode being emitted.
It does so by parsing and compiling Javascript to bytecode,
constructing the same data structure in the testcase and then running a
textual diff between the expected (i.e. the one encoded in the unit test)
and actual (i.e. one built from the compiler output) representation.
This commit is a first step in this direction, achieving just the first
half of what we desire. At the moment, bytecodechecker can:
* take a code snippet from the command line and emit the expected structure.
* adhere to the same formatting rules of the test cases
(this one is important for text diff and for copy and pasting too)
Still to do:
* parse unit tests:
+ extract code snippets
+ indent the code to match the input test case
+ allow flexibility in the input format
+ try to recognize and work around some macro magic (i.e. REPEAT_127)
* emit the representation of the constant pool and handlers vector
* run a textual diff
BUG=v8:4280
LOG=N
==========
to
==========
[Interpreter] Initial generate-bytecode-expectations implementation.
generate-bytecode-expectations is a tool intended to work
together with test/cctest/test-bytecode-generator.cc in
order to produce a meaningful diff between testcases and
the actual bytecode being emitted.
It does so by parsing and compiling Javascript to bytecode,
constructing the same data structure in the testcase and then running a
textual diff between the expected (i.e. the one encoded in the unit test)
and actual (i.e. one built from the compiler output) representation.
This commit is a first step in this direction, achieving just the first
half of what we desire. At the moment, bytecodechecker can:
* take a code snippet from the command line and emit the expected structure.
* adhere to the same formatting rules of the test cases
(this one is important for text diff and for copy and pasting too)
Still to do:
* parse unit tests:
+ extract code snippets
+ indent the code to match the input test case
+ allow flexibility in the input format
+ try to recognize and work around some macro magic (i.e. REPEAT_127)
* emit the representation of the constant pool and handlers vector
* run a textual diff
BUG=v8:4280
LOG=N
==========
Description was changed from
==========
[Interpreter] Initial generate-bytecode-expectations implementation.
generate-bytecode-expectations is a tool intended to work
together with test/cctest/test-bytecode-generator.cc in
order to produce a meaningful diff between testcases and
the actual bytecode being emitted.
It does so by parsing and compiling Javascript to bytecode,
constructing the same data structure in the testcase and then running a
textual diff between the expected (i.e. the one encoded in the unit test)
and actual (i.e. one built from the compiler output) representation.
This commit is a first step in this direction, achieving just the first
half of what we desire. At the moment, bytecodechecker can:
* take a code snippet from the command line and emit the expected structure.
* adhere to the same formatting rules of the test cases
(this one is important for text diff and for copy and pasting too)
Still to do:
* parse unit tests:
+ extract code snippets
+ indent the code to match the input test case
+ allow flexibility in the input format
+ try to recognize and work around some macro magic (i.e. REPEAT_127)
* emit the representation of the constant pool and handlers vector
* run a textual diff
BUG=v8:4280
LOG=N
==========
to
==========
[Interpreter] Initial generate-bytecode-expectations implementation.
generate-bytecode-expectations is a tool intended to work together
with test/cctest/test-bytecode-generator.cc in order to produce a
meaningful diff between testcases and the actual bytecode being emitted.
It does so by parsing and compiling Javascript to bytecode,
constructing the same data structure in the testcase and then running a
textual diff between the expected (i.e. the one encoded in the unit test)
and actual (i.e. one built from the compiler output) representation.
This commit is a first step in this direction, achieving just the first
half of what we desire. At the moment, bytecodechecker can:
* take a code snippet from the command line and emit the expected structure.
* adhere to the same formatting rules of the test cases
(this one is important for text diff and for copy and pasting too)
Still to do:
* parse unit tests:
+ extract code snippets
+ indent the code to match the input test case
+ allow flexibility in the input format
+ try to recognize and work around some macro magic (i.e. REPEAT_127)
* emit the representation of the constant pool and handlers vector
* run a textual diff
BUG=v8:4280
LOG=N
==========
Description was changed from
==========
[Interpreter] Initial generate-bytecode-expectations implementation.
generate-bytecode-expectations is a tool intended to work together
with test/cctest/test-bytecode-generator.cc in order to produce a
meaningful diff between testcases and the actual bytecode being emitted.
It does so by parsing and compiling Javascript to bytecode,
constructing the same data structure in the testcase and then running a
textual diff between the expected (i.e. the one encoded in the unit test)
and actual (i.e. one built from the compiler output) representation.
This commit is a first step in this direction, achieving just the first
half of what we desire. At the moment, bytecodechecker can:
* take a code snippet from the command line and emit the expected structure.
* adhere to the same formatting rules of the test cases
(this one is important for text diff and for copy and pasting too)
Still to do:
* parse unit tests:
+ extract code snippets
+ indent the code to match the input test case
+ allow flexibility in the input format
+ try to recognize and work around some macro magic (i.e. REPEAT_127)
* emit the representation of the constant pool and handlers vector
* run a textual diff
BUG=v8:4280
LOG=N
==========
to
==========
[Interpreter] Initial generate-bytecode-expectations implementation.
generate-bytecode-expectations is a tool intended to work together
with test/cctest/test-bytecode-generator.cc in order to produce a
meaningful diff between testcases and the actual bytecode being emitted.
It does so by parsing and compiling Javascript to bytecode,
constructing the same data structure in the testcase and then running a
textual diff between the expected (i.e. the one encoded in the unit test)
and actual (i.e. the one built from the compiler output) representation.
This commit is a first step in this direction, achieving just the first
half of what we desire. At the moment, bytecodechecker can:
* take a code snippet from the command line and emit the expected structure.
* adhere to the same formatting rules of the test cases
(this one is important for text diff and for copy and pasting too)
Still to do:
* parse unit tests:
+ extract code snippets
+ indent the code to match the input test case
+ allow flexibility in the input format
+ try to recognize and work around some macro magic (i.e. REPEAT_127)
* emit the representation of the constant pool and handlers vector
* run a textual diff
BUG=v8:4280
LOG=N
==========
I finally got git cl format to stop moving stars around. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker.cc File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:21: void *Allocate(size_t length) override { On 2016/02/09 11:58:24, rmcilroy wrote: > On 2016/02/09 10:09:58, ssanfilippo wrote: > > On 2016/02/08 13:55:31, rmcilroy wrote: > > > On 2016/02/08 13:18:34, ssanfilippo wrote: > > > > On 2016/02/05 11:41:05, oth wrote: > > > > > The predominant style in v8 is > > > > > "void*" rather than "void *" and ditto reference arguments throughout > this > > > > file. > > > > > The Google C++ coding style permits either though. > > > > > > > > Done. > > > > > > Doesn't look like this has been done. > > > > It was, but cl format reverted to this style before upload (and I didn't > > notice). Since both forms are acceptable according to the style guide, we > might > > just leave it as it is. If you prefer, I can do a cl upload without cl format. > > > Hmm that's weird (git cl format doesn't do that for me). Did you replace all > instances (including the char* kIndent at the top of the file)? Maybe git cl > format takes the first instance it sees and makes the rest of the file > consistent. I'd still prefer this to be the same as the rest of the V8 style, > but if you can't convince git cl format to play nice then I guess I'll live with > it.§ I finally did it on the workstation. Maybe it's a Mac issue? https://codereview.chromium.org/1671863002/diff/80001/test/cctest/bytecodeche... File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/80001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:148: if (op_size != OperandSize::kByte) stream << size_tag; On 2016/02/09 11:40:34, oth wrote: > At some point we should make this consistent, R->R8. Yes, anytime. Whenever we do the change, this is a trivial fix. https://codereview.chromium.org/1671863002/diff/80001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:178: return trimmed; On 2016/02/09 11:40:34, oth wrote: > Nit. This would be slightly more succinct if it didn't copy source, but just > found the tail position in source and returned std::string(source.begin(), > tail); I agree with you, anyway this function has been removed so it's not an issue anymore. https://codereview.chromium.org/1671863002/diff/80001/test/cctest/bytecodeche... test/cctest/bytecodechecker.cc:211: stream << kIndent << "// ExpectedSnippet generated by bytecodechecker.\n"; On 2016/02/09 11:58:24, rmcilroy wrote: > nit - add "===" header to make this obvious, i.e., "// === ExpectedSnippet > autogenerated by <toolname> ===" Done.
Looks great, lgtm. Thanks. https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker.cc File test/cctest/bytecodechecker.cc (right): https://codereview.chromium.org/1671863002/diff/1/test/cctest/bytecodechecker... test/cctest/bytecodechecker.cc:21: void *Allocate(size_t length) override { On 2016/02/09 16:52:50, ssanfilippo wrote: > On 2016/02/09 11:58:24, rmcilroy wrote: > > On 2016/02/09 10:09:58, ssanfilippo wrote: > > > On 2016/02/08 13:55:31, rmcilroy wrote: > > > > On 2016/02/08 13:18:34, ssanfilippo wrote: > > > > > On 2016/02/05 11:41:05, oth wrote: > > > > > > The predominant style in v8 is > > > > > > "void*" rather than "void *" and ditto reference arguments throughout > > this > > > > > file. > > > > > > The Google C++ coding style permits either though. > > > > > > > > > > Done. > > > > > > > > Doesn't look like this has been done. > > > > > > It was, but cl format reverted to this style before upload (and I didn't > > > notice). Since both forms are acceptable according to the style guide, we > > might > > > just leave it as it is. If you prefer, I can do a cl upload without cl > format. > > > > > > Hmm that's weird (git cl format doesn't do that for me). Did you replace all > > instances (including the char* kIndent at the top of the file)? Maybe git cl > > format takes the first instance it sees and makes the rest of the file > > consistent. I'd still prefer this to be the same as the rest of the V8 style, > > but if you can't convince git cl format to play nice then I guess I'll live > with > > it.§ > > I finally did it on the workstation. Maybe it's a Mac issue? Sounds like it - weird. thanks! https://codereview.chromium.org/1671863002/diff/180001/test/cctest/generate-b... File test/cctest/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1671863002/diff/180001/test/cctest/generate-b... test/cctest/generate-bytecode-expectations.cc:81: v8::Local<v8::String> MakeV8LocalStringFromUTF8(v8::Isolate* isolate, nit - V8StringFromUTF8 https://codereview.chromium.org/1671863002/diff/180001/test/cctest/generate-b... test/cctest/generate-bytecode-expectations.cc:88: const std::string& source_file) { /s/source_file/function_body
https://codereview.chromium.org/1671863002/diff/180001/test/cctest/generate-b... File test/cctest/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1671863002/diff/180001/test/cctest/generate-b... test/cctest/generate-bytecode-expectations.cc:81: v8::Local<v8::String> MakeV8LocalStringFromUTF8(v8::Isolate* isolate, On 2016/02/10 09:18:00, rmcilroy wrote: > nit - V8StringFromUTF8 Done. https://codereview.chromium.org/1671863002/diff/180001/test/cctest/generate-b... test/cctest/generate-bytecode-expectations.cc:88: const std::string& source_file) { On 2016/02/10 09:18:00, rmcilroy wrote: > /s/source_file/function_body Done.
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/1671863002/#ps200001 (title: "Rename V8StringFromUTF8 and function_body")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1671863002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1671863002/200001
The CQ bit was unchecked by ssanfilippo@chromium.org
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/1671863002/#ps220001 (title: "Moved to interpreter/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1671863002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1671863002/220001
Message was sent while issue was closed.
Description was changed from
==========
[Interpreter] Initial generate-bytecode-expectations implementation.
generate-bytecode-expectations is a tool intended to work together
with test/cctest/test-bytecode-generator.cc in order to produce a
meaningful diff between testcases and the actual bytecode being emitted.
It does so by parsing and compiling Javascript to bytecode,
constructing the same data structure in the testcase and then running a
textual diff between the expected (i.e. the one encoded in the unit test)
and actual (i.e. the one built from the compiler output) representation.
This commit is a first step in this direction, achieving just the first
half of what we desire. At the moment, bytecodechecker can:
* take a code snippet from the command line and emit the expected structure.
* adhere to the same formatting rules of the test cases
(this one is important for text diff and for copy and pasting too)
Still to do:
* parse unit tests:
+ extract code snippets
+ indent the code to match the input test case
+ allow flexibility in the input format
+ try to recognize and work around some macro magic (i.e. REPEAT_127)
* emit the representation of the constant pool and handlers vector
* run a textual diff
BUG=v8:4280
LOG=N
==========
to
==========
[Interpreter] Initial generate-bytecode-expectations implementation.
generate-bytecode-expectations is a tool intended to work together
with test/cctest/test-bytecode-generator.cc in order to produce a
meaningful diff between testcases and the actual bytecode being emitted.
It does so by parsing and compiling Javascript to bytecode,
constructing the same data structure in the testcase and then running a
textual diff between the expected (i.e. the one encoded in the unit test)
and actual (i.e. the one built from the compiler output) representation.
This commit is a first step in this direction, achieving just the first
half of what we desire. At the moment, bytecodechecker can:
* take a code snippet from the command line and emit the expected structure.
* adhere to the same formatting rules of the test cases
(this one is important for text diff and for copy and pasting too)
Still to do:
* parse unit tests:
+ extract code snippets
+ indent the code to match the input test case
+ allow flexibility in the input format
+ try to recognize and work around some macro magic (i.e. REPEAT_127)
* emit the representation of the constant pool and handlers vector
* run a textual diff
BUG=v8:4280
LOG=N
==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from
==========
[Interpreter] Initial generate-bytecode-expectations implementation.
generate-bytecode-expectations is a tool intended to work together
with test/cctest/test-bytecode-generator.cc in order to produce a
meaningful diff between testcases and the actual bytecode being emitted.
It does so by parsing and compiling Javascript to bytecode,
constructing the same data structure in the testcase and then running a
textual diff between the expected (i.e. the one encoded in the unit test)
and actual (i.e. the one built from the compiler output) representation.
This commit is a first step in this direction, achieving just the first
half of what we desire. At the moment, bytecodechecker can:
* take a code snippet from the command line and emit the expected structure.
* adhere to the same formatting rules of the test cases
(this one is important for text diff and for copy and pasting too)
Still to do:
* parse unit tests:
+ extract code snippets
+ indent the code to match the input test case
+ allow flexibility in the input format
+ try to recognize and work around some macro magic (i.e. REPEAT_127)
* emit the representation of the constant pool and handlers vector
* run a textual diff
BUG=v8:4280
LOG=N
==========
to
==========
[Interpreter] Initial generate-bytecode-expectations implementation.
generate-bytecode-expectations is a tool intended to work together
with test/cctest/test-bytecode-generator.cc in order to produce a
meaningful diff between testcases and the actual bytecode being emitted.
It does so by parsing and compiling Javascript to bytecode,
constructing the same data structure in the testcase and then running a
textual diff between the expected (i.e. the one encoded in the unit test)
and actual (i.e. the one built from the compiler output) representation.
This commit is a first step in this direction, achieving just the first
half of what we desire. At the moment, bytecodechecker can:
* take a code snippet from the command line and emit the expected structure.
* adhere to the same formatting rules of the test cases
(this one is important for text diff and for copy and pasting too)
Still to do:
* parse unit tests:
+ extract code snippets
+ indent the code to match the input test case
+ allow flexibility in the input format
+ try to recognize and work around some macro magic (i.e. REPEAT_127)
* emit the representation of the constant pool and handlers vector
* run a textual diff
BUG=v8:4280
LOG=N
Committed: https://crrev.com/d3604cdb6808789e5f60b3f56f0ec2e992fad56c
Cr-Commit-Position: refs/heads/master@{#33863}
==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/d3604cdb6808789e5f60b3f56f0ec2e992fad56c Cr-Commit-Position: refs/heads/master@{#33863} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
