Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1688383003:
[Interpreter] Change the output format of generate-bytecode-expectations. (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] Change the output format of generate-bytecode-expectations.
Now the tool produces a far more readable output format, which bears a
lot of resemblance to YAML. In fact, the output should be machine
parseable as such, one document per testcase. However, the output format
may be subject to changes in future, so don't rely on this property.
In general, the output format has been optimized for producing a meaningful
textual diff, while keeping a decent readability as well. Therefore, not
everything is as compact as it could be, e.g. for an empty const pool we get:
constant pool: [
]
instead of:
constant pool: []
Also, trailing commas are always inserted in lists.
Additionally, now the tool accepts its output format as input. When
operating in this mode, all the snippets are extracted, processed and
the output is then emitted as usual. If nothing has changed, the output
should match the input. This is very useful for catching bugs in the
bytecode generation by running a textual diff against a known-good file.
The core (namely bytecode-expectations.cc) has been extracted from the
original cc file, which provides the utility as usual. The definitions
in the matching header of the library have been moved into the
v8::internal::interpreter namespace.
The library exposes a class ExpectationPrinter, with a method
PrintExpectation, which takes a test snippet as input, and writes the
formatted expectation to the supplied stream. One might then use a
std::stringstream to retrieve the results as a string and run it through
a diff utility.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/e082ebdbf3f352d55850c87dff3d5d01d772655b
Cr-Commit-Position: refs/heads/master@{#33997}
Patch Set 1 #Patch Set 2 : Do not create a static lib target. #
Total comments: 17
Patch Set 3 : Change bytecode-expectations API. #Patch Set 4 : Address reviewers' comments. #Patch Set 5 : Fix help message. #
Total comments: 42
Patch Set 6 : #Patch Set 7 : Second round of fixes. #Patch Set 8 : Rename program options, move methods out of class. #
Total comments: 26
Patch Set 9 : Third round of reviews + fix all lint warnings. #Patch Set 10 : Simplify indent stripping with a CHECK, rebase changed cctest.gyp #Patch Set 11 : Fix Clang on Mac OS X compiler error. #Patch Set 12 : Fix MSVC on Windows compiler error. #
Messages
Total messages: 43 (23 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Interpreter] Change the output format of generate-bytecode-expectations. Now the tool produces a far more readable output format, which bears a lot of resemblance to YAML. In fact, the output should be machine parseable as such, one document per testcase. However, the output format may be subject to changes in future, so don't rely too much on this property. Additionally, now the tool accepts its output format as input. When operating in this mode, all the snippets are extracted, processed and the output is then emitted as usual. If nothing has changed, the output should match the input. This is very useful for catching bugs in the bytecode generation by running a textual diff against a known-good file. Additionally, the file has been split between a header, which can be used as a small static library, and the cc file, which provides the utility as usual. The functions in the header have been moved into the v8::interpreter::internal namespace. The relevant call from the header is GenerateExpectationsFile, which takes a vector of test snippets as input, and writes the output to the supplied stream. One might use a std::stringstream to retrieve the results as string and perform some sort of text diffing. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Change the output format of generate-bytecode-expectations. Now the tool produces a far more readable output format, which bears a lot of resemblance to YAML. In fact, the output should be machine parseable as such, one document per testcase. However, the output format may be subject to changes in future, so don't rely too much on this property. Additionally, now the tool accepts its output format as input. When operating in this mode, all the snippets are extracted, processed and the output is then emitted as usual. If nothing has changed, the output should match the input. This is very useful for catching bugs in the bytecode generation by running a textual diff against a known-good file. A library (namely bytecode-expectations) has been extracted from the original cc file, which provides the utility as usual. The definitions in the library header have been moved into the v8::interpreter::internal namespace. The relevant call from the library is GenerateExpectationsFile, which takes a vector of test snippets as input, and writes the output to the supplied stream. One might use a std::stringstream to retrieve the results as string and perform some sort of text diffing. BUG=v8:4280 LOG=N ==========
ssanfilippo@chromium.org changed reviewers: + oth@chromium.org, rmccool@google.com
ssanfilippo@chromium.org changed reviewers: + rmcilroy@chromium.org - rmccool@google.com
Here is the same utility, with a better output format. The core has been moved to a separate library, which can be reused e.g. in unit tests. Turns out having a separate static library target is both easy to achieve and clearer, since the core itself depends on V8 internals and thus require a not-super-straightforward rule for linking against the snapshot. Otherwise, we get a lot of complaints from the linker because of declspecs. If one is willing to reuse the lib in a test, just add bytecode-expectations to the dependencies of the target and include test/cctest/bytecode-expectations.h in the source. PTAL.
Description was changed from ========== [Interpreter] Change the output format of generate-bytecode-expectations. Now the tool produces a far more readable output format, which bears a lot of resemblance to YAML. In fact, the output should be machine parseable as such, one document per testcase. However, the output format may be subject to changes in future, so don't rely too much on this property. Additionally, now the tool accepts its output format as input. When operating in this mode, all the snippets are extracted, processed and the output is then emitted as usual. If nothing has changed, the output should match the input. This is very useful for catching bugs in the bytecode generation by running a textual diff against a known-good file. A library (namely bytecode-expectations) has been extracted from the original cc file, which provides the utility as usual. The definitions in the library header have been moved into the v8::interpreter::internal namespace. The relevant call from the library is GenerateExpectationsFile, which takes a vector of test snippets as input, and writes the output to the supplied stream. One might use a std::stringstream to retrieve the results as string and perform some sort of text diffing. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Change the output format of generate-bytecode-expectations. Now the tool produces a far more readable output format, which bears a lot of resemblance to YAML. In fact, the output should be machine parseable as such, one document per testcase. However, the output format may be subject to changes in future, so don't rely too much on this property. Additionally, now the tool accepts its output format as input. When operating in this mode, all the snippets are extracted, processed and the output is then emitted as usual. If nothing has changed, the output should match the input. This is very useful for catching bugs in the bytecode generation by running a textual diff against a known-good file. The core (namely bytecode-expectations.cc) has been extracted from the original cc file, which provides the utility as usual. The definitions in the matching header of the library have been moved into the v8::interpreter::internal namespace. The relevant call from the library is GenerateExpectationsFile, which takes a vector of test snippets as input, and writes the output to the supplied stream. One might use a std::stringstream to retrieve the results as string and perform some sort of text diffing. BUG=v8:4280 LOG=N ==========
https://codereview.chromium.org/1688383003/diff/40001/test/cctest/cctest.gyp File test/cctest/cctest.gyp (right): https://codereview.chromium.org/1688383003/diff/40001/test/cctest/cctest.gyp#... test/cctest/cctest.gyp:365: '../../test/cctest/interpreter/bytecode-expectations.cc', Also include the .h file https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... File test/cctest/interpreter/bytecode-expectations.h (right): https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/bytecode-expectations.h:24: std::string snippet; Fields go at the end. Also append "_" to the end of field names. I think this should also be a class with the fields being private and having public getters. https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/bytecode-expectations.h:31: void GenerateExpectationsFile(std::ostream& stream, As discussed, let's refactor this into a class and move the V8 initialization stuff out intp the generate-bytecode-expections.cc file.
https://codereview.chromium.org/1688383003/diff/40001/test/cctest/cctest.gyp File test/cctest/cctest.gyp (right): https://codereview.chromium.org/1688383003/diff/40001/test/cctest/cctest.gyp#... test/cctest/cctest.gyp:365: '../../test/cctest/interpreter/bytecode-expectations.cc', On 2016/02/12 12:33:10, rmcilroy wrote: > Also include the .h file Done. https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... File test/cctest/interpreter/bytecode-expectations.h (right): https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/bytecode-expectations.h:24: std::string snippet; On 2016/02/12 12:33:10, rmcilroy wrote: > Fields go at the end. Also append "_" to the end of field names. I think this > should also be a class with the fields being private and having public getters. This struct was removed in the refactoring. https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/bytecode-expectations.h:31: void GenerateExpectationsFile(std::ostream& stream, On 2016/02/12 12:33:10, rmcilroy wrote: > As discussed, let's refactor this into a class and move the V8 initialization > stuff out intp the generate-bytecode-expections.cc file. Done.
Description was changed from ========== [Interpreter] Change the output format of generate-bytecode-expectations. Now the tool produces a far more readable output format, which bears a lot of resemblance to YAML. In fact, the output should be machine parseable as such, one document per testcase. However, the output format may be subject to changes in future, so don't rely too much on this property. Additionally, now the tool accepts its output format as input. When operating in this mode, all the snippets are extracted, processed and the output is then emitted as usual. If nothing has changed, the output should match the input. This is very useful for catching bugs in the bytecode generation by running a textual diff against a known-good file. The core (namely bytecode-expectations.cc) has been extracted from the original cc file, which provides the utility as usual. The definitions in the matching header of the library have been moved into the v8::interpreter::internal namespace. The relevant call from the library is GenerateExpectationsFile, which takes a vector of test snippets as input, and writes the output to the supplied stream. One might use a std::stringstream to retrieve the results as string and perform some sort of text diffing. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Change the output format of generate-bytecode-expectations. Now the tool produces a far more readable output format, which bears a lot of resemblance to YAML. In fact, the output should be machine parseable as such, one document per testcase. However, the output format may be subject to changes in future, so don't rely too much on this property. Additionally, now the tool accepts its output format as input. When operating in this mode, all the snippets are extracted, processed and the output is then emitted as usual. If nothing has changed, the output should match the input. This is very useful for catching bugs in the bytecode generation by running a textual diff against a known-good file. The core (namely bytecode-expectations.cc) has been extracted from the original cc file, which provides the utility as usual. The definitions in the matching header of the library have been moved into the v8::interpreter::internal namespace. The library exposes a class ExpectationPrinter, with a method PrintExpectation, which takes a test snippet as input, and writes the formatted expectation to the supplied stream. One might then use a std::stringstream to retrieve the results as string and perform a textual diffing. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Change the output format of generate-bytecode-expectations. Now the tool produces a far more readable output format, which bears a lot of resemblance to YAML. In fact, the output should be machine parseable as such, one document per testcase. However, the output format may be subject to changes in future, so don't rely too much on this property. Additionally, now the tool accepts its output format as input. When operating in this mode, all the snippets are extracted, processed and the output is then emitted as usual. If nothing has changed, the output should match the input. This is very useful for catching bugs in the bytecode generation by running a textual diff against a known-good file. The core (namely bytecode-expectations.cc) has been extracted from the original cc file, which provides the utility as usual. The definitions in the matching header of the library have been moved into the v8::interpreter::internal namespace. The library exposes a class ExpectationPrinter, with a method PrintExpectation, which takes a test snippet as input, and writes the formatted expectation to the supplied stream. One might then use a std::stringstream to retrieve the results as string and perform a textual diffing. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Change the output format of generate-bytecode-expectations. Now the tool produces a far more readable output format, which bears a lot of resemblance to YAML. In fact, the output should be machine parseable as such, one document per testcase. However, the output format may be subject to changes in future, so don't rely too much on this property. Additionally, now the tool accepts its output format as input. When operating in this mode, all the snippets are extracted, processed and the output is then emitted as usual. If nothing has changed, the output should match the input. This is very useful for catching bugs in the bytecode generation by running a textual diff against a known-good file. The core (namely bytecode-expectations.cc) has been extracted from the original cc file, which provides the utility as usual. The definitions in the matching header of the library have been moved into the v8::interpreter::internal namespace. The library exposes a class ExpectationPrinter, with a method PrintExpectation, which takes a test snippet as input, and writes the formatted expectation to the supplied stream. One might then use a std::stringstream to retrieve the results as a string and run it through a textual diffing utility. BUG=v8:4280 LOG=N ==========
A few minor comments, but looking good. https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:46: bool ReadNextSnippet(std::string* string, std::istream& stream) { Optional nit: std::ostringstream might be a better match here. https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:57: // Skip two leading spaces we added, if the line is not shorter Consider just skipping whitespace here (string::find_first_not_of). The magic 2 seems a bit fragile, ie someone might edit a file and have tabs set to 2. https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:155: " --js Read raw JavaScript, instead of my output format " s/my output/the default/ https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:156: "(default: no).\n" Do not need (default:no) here. https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:157: " --stdin Read from standard input instead of file (default: no).\n" Do not need (default:no) here.
Description was changed from
==========
[Interpreter] Change the output format of generate-bytecode-expectations.
Now the tool produces a far more readable output format, which bears a
lot of resemblance to YAML. In fact, the output should be machine
parseable as such, one document per testcase. However, the output format
may be subject to changes in future, so don't rely too much on this
property.
Additionally, now the tool accepts its output format as input. When
operating in this mode, all the snippets are extracted, processed and
the output is then emitted as usual. If nothing has changed, the output
should match the input. This is very useful for catching bugs in the
bytecode generation by running a textual diff against a known-good file.
The core (namely bytecode-expectations.cc) has been extracted from the
original cc file, which provides the utility as usual. The definitions
in the matching header of the library have been moved into the
v8::interpreter::internal namespace.
The library exposes a class ExpectationPrinter, with a method
PrintExpectation, which takes a test snippet as input, and writes the
formatted expectation to the supplied stream. One might then use a
std::stringstream to retrieve the results as a string and run it through
a textual diffing utility.
BUG=v8:4280
LOG=N
==========
to
==========
[Interpreter] Change the output format of generate-bytecode-expectations.
Now the tool produces a far more readable output format, which bears a
lot of resemblance to YAML. In fact, the output should be machine
parseable as such, one document per testcase. However, the output format
may be subject to changes in future, so don't rely on this property.
In general, the output format has been optimized for meaningful diffing,
while keeping a decent readability, so not everything is as compact as
it could be, e.g. for an empty const pool we get:
constant pool: [
]
instead of:
constant pool: []
Also, trailing commas are always inserted in lists.
Additionally, now the tool accepts its output format as input. When
operating in this mode, all the snippets are extracted, processed and
the output is then emitted as usual. If nothing has changed, the output
should match the input. This is very useful for catching bugs in the
bytecode generation by running a textual diff against a known-good file.
The core (namely bytecode-expectations.cc) has been extracted from the
original cc file, which provides the utility as usual. The definitions
in the matching header of the library have been moved into the
v8::interpreter::internal namespace.
The library exposes a class ExpectationPrinter, with a method
PrintExpectation, which takes a test snippet as input, and writes the
formatted expectation to the supplied stream. One might then use a
std::stringstream to retrieve the results as a string and run it through
a textual diffing utility.
BUG=v8:4280
LOG=N
==========
https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:46: bool ReadNextSnippet(std::string* string, std::istream& stream) { On 2016/02/12 14:40:14, oth wrote: > Optional nit: std::ostringstream might be a better match here. The snippets then get pushed into a vector as strings at line 157. I'd prefer to keep this signature since I think it is clearer given the use of the function. https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:57: // Skip two leading spaces we added, if the line is not shorter On 2016/02/12 14:40:14, oth wrote: > Consider just skipping whitespace here (string::find_first_not_of). The magic 2 > seems a bit fragile, ie someone might edit a file and have tabs set to 2. The issue is that we want the output to exactly match the input, so if we remove all leading spaces, we might end up emitting something that differs in the placing of spaces in the output. The 2 spaces in questions are added at line 201 of bytecodes-expectations.cc. Anyway, I agree it could be more robust, but I am not sure how we could tackle this issue. https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:155: " --js Read raw JavaScript, instead of my output format " On 2016/02/12 14:40:13, oth wrote: > s/my output/the default/ Done. https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:156: "(default: no).\n" On 2016/02/12 14:40:14, oth wrote: > Do not need (default:no) here. Done. https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:157: " --stdin Read from standard input instead of file (default: no).\n" On 2016/02/12 14:40:14, oth wrote: > Do not need (default:no) here. Done.
https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... File test/cctest/interpreter/bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.cc:25: i::Isolate* GetInternalIsolate(v8::Isolate* isolate) { Move this to generate-bytecode-expectations.cc and just pass an internal isolate to this class. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.cc:87: void PrintBytecodeOperand(std::ostream& stream, Functions from here onwards should be private functions in BytecodeExpectationPrinter I think. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... File test/cctest/interpreter/bytecode-expectations.h (right): https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.h:5: #ifndef INTERPRETER_BYTECODE_EXPECTATIONS_H_ V8_INTERPRETER_BYTECODE_EXPECTATIONS_PRINTER_H_ (after file rename below) https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.h:18: enum ConstantPoolType { Pull into BytecodeExpectationsPrinter as an inner emum https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.h:26: class ExpectationPrinter final { Please rename file to bytecode-expectations-printer and this class to BytecodeExpectationsPrinter https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.h:33: void SetConstantPoolType(ConstantPoolType t) { const_pool_type_ = t; } set_constant_pool_type for a setter https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.h:34: ConstantPoolType GetConstantPoolType() const { return const_pool_type_; } constant_pool_type for a getter (drop the Get) https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.h:45: #endif #endif // V8_INTERPRETER_BYTECODE_EXPECTATIONS_PRINTER_H_ https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:16: rm extra newline https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:23: struct CommandLineOptions { Make this a class if it has non-neglagible functionality. Also move fields to the bottom, make them private, and add "_" to their name and add getters. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:25: bool read_raw_snippet = false; read_raw_js_snippet https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:27: bool read_from_stdin = false; nit - move up with other bools https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:102: bool ReadAll(std::string* body, std::istream& stream) { ReadRawJsSnippet https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:109: bool ReadNextSnippet(std::string* string, std::istream& stream) { Just return the string instead of having it as a out pointer, and check for end of file in the while loop in ExtractSnippets (with a CHECK or UNREACHABLE assert here if the format is not as expected) https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:131: bool ExtractSnippets(std::vector<std::string>* snippet_list, const std::vector<std::string>* https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:179: ExpectationPrinter printer{platform.isolate(), const_pool_type}; replace "{" with "(" https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:199: CommandLineOptions ParseOptionsFromCommandLine(int argc, char** argv) { This should probably be a constructor for CommandLineOptions, or a static Create method https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:208: if (options.const_pool_type == kConstantPoolTypeUnknown) { Move this logic into CommandLineArguments::Validate https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:235: << " [OPTIONS]... [INPUT FILES]...\n\n" Let's make this tool only take a single file - the use case would be fixing a golden expectation file and replacing it, so I don't see much usecase for multiple files (which just complicates thing IMO). https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:238: " --js Read raw JavaScript, instead of the output format.\n" nit / --raw-js
https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:57: // Skip two leading spaces we added, if the line is not shorter On 2016/02/12 14:49:52, ssanfilippo wrote: > On 2016/02/12 14:40:14, oth wrote: > > Consider just skipping whitespace here (string::find_first_not_of). The magic > 2 > > seems a bit fragile, ie someone might edit a file and have tabs set to 2. > > The issue is that we want the output to exactly match the input, so if we remove > all leading spaces, we might end up emitting something that differs in the > placing of spaces in the output. > > The 2 spaces in questions are added at line 201 of bytecodes-expectations.cc. > Anyway, I agree it could be more robust, but I am not sure how we could tackle > this issue. I don't think keeping the output matching the input should be a concern - we own the output format now since it will be always generated by this tool - don't overcomplicate the parsing to allow arbitrary input, just assume the input was generated by this same tool.
https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... File test/cctest/interpreter/bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.cc:25: i::Isolate* GetInternalIsolate(v8::Isolate* isolate) { On 2016/02/12 15:29:05, rmcilroy wrote: > Move this to generate-bytecode-expectations.cc and just pass an internal isolate > to this class. Turns out we need the external Isolate as well, for calls like GetCurrentContext() and Compile(). Shall I do the opposite cast we needed? https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.cc:87: void PrintBytecodeOperand(std::ostream& stream, On 2016/02/12 15:29:05, rmcilroy wrote: > Functions from here onwards should be private functions in > BytecodeExpectationPrinter I think. Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... File test/cctest/interpreter/bytecode-expectations.h (right): https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.h:5: #ifndef INTERPRETER_BYTECODE_EXPECTATIONS_H_ On 2016/02/12 15:29:05, rmcilroy wrote: > V8_INTERPRETER_BYTECODE_EXPECTATIONS_PRINTER_H_ (after file rename below) Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.h:18: enum ConstantPoolType { On 2016/02/12 15:29:05, rmcilroy wrote: > Pull into BytecodeExpectationsPrinter as an inner emum Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.h:26: class ExpectationPrinter final { On 2016/02/12 15:29:05, rmcilroy wrote: > Please rename file to bytecode-expectations-printer and this class to > BytecodeExpectationsPrinter Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.h:33: void SetConstantPoolType(ConstantPoolType t) { const_pool_type_ = t; } On 2016/02/12 15:29:05, rmcilroy wrote: > set_constant_pool_type for a setter Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.h:34: ConstantPoolType GetConstantPoolType() const { return const_pool_type_; } On 2016/02/12 15:29:05, rmcilroy wrote: > constant_pool_type for a getter (drop the Get) Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.h:45: #endif On 2016/02/12 15:29:05, rmcilroy wrote: > #endif // V8_INTERPRETER_BYTECODE_EXPECTATIONS_PRINTER_H_ Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:16: On 2016/02/12 15:29:05, rmcilroy wrote: > rm extra newline Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:23: struct CommandLineOptions { On 2016/02/12 15:29:05, rmcilroy wrote: > Make this a class if it has non-neglagible functionality. Also move fields to > the bottom, make them private, and add "_" to their name and add getters. Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:25: bool read_raw_snippet = false; On 2016/02/12 15:29:05, rmcilroy wrote: > read_raw_js_snippet Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:27: bool read_from_stdin = false; On 2016/02/12 15:29:05, rmcilroy wrote: > nit - move up with other bools Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:102: bool ReadAll(std::string* body, std::istream& stream) { On 2016/02/12 15:29:05, rmcilroy wrote: > ReadRawJsSnippet Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:109: bool ReadNextSnippet(std::string* string, std::istream& stream) { On 2016/02/12 15:29:05, rmcilroy wrote: > Just return the string instead of having it as a out pointer, and check for end > of file in the while loop in ExtractSnippets (with a CHECK or UNREACHABLE assert > here if the format is not as expected) The fact is that EOF might test true after that function even if we read a valid snippet, because that snippet was the very last thing in the file. The boolean flag makes this difference through the two possible exit paths from the function. So, either we make the assumption that this is never going to happen, or we should keep this IMO. The other possibility would be to return a string and the success flag as an output pointer, but this would just complicate the while loop below, again IMHO. We might also want to use the empty string as a condition to break the loop, but there's a test case whose input is the empty string itself, which anyway is a valid snippet. I am updating the fix to all the other issues but this one since I feel we might want to talk about it further, hope you don't mind. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:131: bool ExtractSnippets(std::vector<std::string>* snippet_list, On 2016/02/12 15:29:05, rmcilroy wrote: > const std::vector<std::string>* We are modifying the vector in the function, const won't do :) https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:179: ExpectationPrinter printer{platform.isolate(), const_pool_type}; On 2016/02/12 15:29:05, rmcilroy wrote: > replace "{" with "(" Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:199: CommandLineOptions ParseOptionsFromCommandLine(int argc, char** argv) { On 2016/02/12 15:29:05, rmcilroy wrote: > This should probably be a constructor for CommandLineOptions, or a static Create > method Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:208: if (options.const_pool_type == kConstantPoolTypeUnknown) { On 2016/02/12 15:29:05, rmcilroy wrote: > Move this logic into CommandLineArguments::Validate Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:235: << " [OPTIONS]... [INPUT FILES]...\n\n" On 2016/02/12 15:29:05, rmcilroy wrote: > Let's make this tool only take a single file - the use case would be fixing a > golden expectation file and replacing it, so I don't see much usecase for > multiple files (which just complicates thing IMO). Done. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:238: " --js Read raw JavaScript, instead of the output format.\n" On 2016/02/12 15:29:05, rmcilroy wrote: > nit / --raw-js Done.
LGTM once comments are addressed. Thanks. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... File test/cctest/interpreter/bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations.cc:25: i::Isolate* GetInternalIsolate(v8::Isolate* isolate) { On 2016/02/12 18:29:40, ssanfilippo wrote: > On 2016/02/12 15:29:05, rmcilroy wrote: > > Move this to generate-bytecode-expectations.cc and just pass an internal > isolate > > to this class. > > Turns out we need the external Isolate as well, for calls like > GetCurrentContext() and Compile(). > Shall I do the opposite cast we needed? What you've done here is fine (modulo the rename of GetInternalIsolate to i_isolate() or similar). https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:109: bool ReadNextSnippet(std::string* string, std::istream& stream) { On 2016/02/12 18:29:40, ssanfilippo wrote: > On 2016/02/12 15:29:05, rmcilroy wrote: > > Just return the string instead of having it as a out pointer, and check for > end > > of file in the while loop in ExtractSnippets (with a CHECK or UNREACHABLE > assert > > here if the format is not as expected) > > The fact is that EOF might test true after that function even if we read a valid > snippet, because that snippet was the very last thing in the file. The boolean > flag makes this difference through the two possible exit paths from the > function. > > So, either we make the assumption that this is never going to happen, or we > should keep this IMO. The other possibility would be to return a string and the > success flag as an output pointer, but this would just complicate the while loop > below, again IMHO. > > We might also want to use the empty string as a condition to break the loop, but > there's a test case whose input is the empty string itself, which anyway is a > valid snippet. > > I am updating the fix to all the other issues but this one since I feel we might > want to talk about it further, hope you don't mind. Ok, this is fine as is https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... File test/cctest/interpreter/bytecode-expectations-printer.cc (right): https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations-printer.cc:27: i::Isolate* BytecodeExpectationsPrinter::GetInternalIsolate() const { nit - just internal_isolate() or even i_isolate() since this is just a getter with a cast (can be in .h file too). https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... File test/cctest/interpreter/bytecode-expectations-printer.h (right): https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations-printer.h:50: const char* global_name) const; nit - move these helpers between the PrintXX functions and the fields (still with whitespace between each). https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:34: } nit - move below V8InitializationScope https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:56: BytecodeExpectationsPrinter::ConstantPoolType::kMixed; Don't use default values in fields, initialize them to their default values in the constructor instead. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:85: /* static */ nit - line comments instead of block comments (i.e. "// static") https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:100: } else if (strcmp(argv[i], "--pool-type") == 0) { Could you just make this a --pool-type=..." argument to avoid having to have the expecting_const_pool_type bool? https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:105: options.filename_ = argv[i]; check options.filename_ is empty beforehand (i.e., only one filename passed) https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:158: GetInternalIsolate(isolate_)->interpreter()->Initialize(); this shouldn't be necessary anymore given some previous updates in the interpreter. If it works without it could you remove it (and remove GetInternalIsolate from this file too). https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:167: std::string ReadRawJSSnippet(std::istream& stream) { All references must be const (https://google.github.io/styleguide/cppguide.html#Reference_Arguments). https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:173: bool ReadNextSnippet(std::string* string, std::istream& stream) { ditto. Also rename string->string_out and put it at the end of the parameter list. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:188: *string += line; What string gets added by this? Maybe just only append lines which are greater than/equal to 2 characters? https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:198: snippet_list->push_back(ReadRawJSSnippet(std::cin)); You can pull this into the code below (e.g., if options.read_from_stdin() then std::ostream = std::cin, else std::ostream = std::ifstream(options.filename()) https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:203: std::ifstream body_file{options.filename()}; Please don't use curly bracket initialization for these callse - use normal brackets (thoughout the CL).
https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... File test/cctest/interpreter/bytecode-expectations-printer.cc (right): https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations-printer.cc:27: i::Isolate* BytecodeExpectationsPrinter::GetInternalIsolate() const { On 2016/02/15 11:15:19, rmcilroy wrote: > nit - just internal_isolate() or even i_isolate() since this is just a getter > with a cast (can be in .h file too). Done. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... File test/cctest/interpreter/bytecode-expectations-printer.h (right): https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/bytecode-expectations-printer.h:50: const char* global_name) const; On 2016/02/15 11:15:19, rmcilroy wrote: > nit - move these helpers between the PrintXX functions and the fields (still > with whitespace between each). Done. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:34: } On 2016/02/15 11:15:19, rmcilroy wrote: > nit - move below V8InitializationScope Done. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:56: BytecodeExpectationsPrinter::ConstantPoolType::kMixed; On 2016/02/15 11:15:19, rmcilroy wrote: > Don't use default values in fields, initialize them to their default values in > the constructor instead. Done. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:85: /* static */ On 2016/02/15 11:15:19, rmcilroy wrote: > nit - line comments instead of block comments (i.e. "// static") Done. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:100: } else if (strcmp(argv[i], "--pool-type") == 0) { On 2016/02/15 11:15:20, rmcilroy wrote: > Could you just make this a --pool-type=..." argument to avoid having to have the > expecting_const_pool_type bool? Done. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:105: options.filename_ = argv[i]; On 2016/02/15 11:15:19, rmcilroy wrote: > check options.filename_ is empty beforehand (i.e., only one filename passed) Done. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:158: GetInternalIsolate(isolate_)->interpreter()->Initialize(); On 2016/02/15 11:15:19, rmcilroy wrote: > this shouldn't be necessary anymore given some previous updates in the > interpreter. If it works without it could you remove it (and remove > GetInternalIsolate from this file too). Done. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:167: std::string ReadRawJSSnippet(std::istream& stream) { On 2016/02/15 11:15:19, rmcilroy wrote: > All references must be const > (https://google.github.io/styleguide/cppguide.html#Reference_Arguments). As discussed, I think it's best if we keep it consistent with how streams are passed around in the C++ standard library. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:173: bool ReadNextSnippet(std::string* string, std::istream& stream) { On 2016/02/15 11:15:20, rmcilroy wrote: > ditto. Also rename string->string_out and put it at the end of the parameter > list. Done. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:188: *string += line; On 2016/02/15 11:15:20, rmcilroy wrote: > What string gets added by this? Maybe just only append lines which are greater > than/equal to 2 characters? This is done to preserve the original format, i.e. if we have blank lines in the input. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:198: snippet_list->push_back(ReadRawJSSnippet(std::cin)); On 2016/02/15 11:15:19, rmcilroy wrote: > You can pull this into the code below (e.g., if options.read_from_stdin() then > std::ostream = std::cin, else std::ostream = std::ifstream(options.filename()) Done, although in a slightly different way because fstreams cannot be assigned. https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:203: std::ifstream body_file{options.filename()}; On 2016/02/15 11:15:19, rmcilroy wrote: > Please don't use curly bracket initialization for these callse - use normal > brackets (thoughout the CL). 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/1688383003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688383003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/15454)
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/1688383003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688383003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_compil...)
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/1688383003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688383003/240001
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/1688383003/#ps240001 (title: "Fix MSVC on Windows compiler error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688383003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688383003/240001
The CQ bit was unchecked by ssanfilippo@chromium.org
Description was changed from
==========
[Interpreter] Change the output format of generate-bytecode-expectations.
Now the tool produces a far more readable output format, which bears a
lot of resemblance to YAML. In fact, the output should be machine
parseable as such, one document per testcase. However, the output format
may be subject to changes in future, so don't rely on this property.
In general, the output format has been optimized for meaningful diffing,
while keeping a decent readability, so not everything is as compact as
it could be, e.g. for an empty const pool we get:
constant pool: [
]
instead of:
constant pool: []
Also, trailing commas are always inserted in lists.
Additionally, now the tool accepts its output format as input. When
operating in this mode, all the snippets are extracted, processed and
the output is then emitted as usual. If nothing has changed, the output
should match the input. This is very useful for catching bugs in the
bytecode generation by running a textual diff against a known-good file.
The core (namely bytecode-expectations.cc) has been extracted from the
original cc file, which provides the utility as usual. The definitions
in the matching header of the library have been moved into the
v8::interpreter::internal namespace.
The library exposes a class ExpectationPrinter, with a method
PrintExpectation, which takes a test snippet as input, and writes the
formatted expectation to the supplied stream. One might then use a
std::stringstream to retrieve the results as a string and run it through
a textual diffing utility.
BUG=v8:4280
LOG=N
==========
to
==========
[Interpreter] Change the output format of generate-bytecode-expectations.
Now the tool produces a far more readable output format, which bears a
lot of resemblance to YAML. In fact, the output should be machine
parseable as such, one document per testcase. However, the output format
may be subject to changes in future, so don't rely on this property.
In general, the output format has been optimized for meaningful diffing,
while keeping a decent readability, so not everything is as compact as
it could be, e.g. for an empty const pool we get:
constant pool: [
]
instead of:
constant pool: []
Also, trailing commas are always inserted in lists.
Additionally, now the tool accepts its output format as input. When
operating in this mode, all the snippets are extracted, processed and
the output is then emitted as usual. If nothing has changed, the output
should match the input. This is very useful for catching bugs in the
bytecode generation by running a textual diff against a known-good file.
The core (namely bytecode-expectations.cc) has been extracted from the
original cc file, which provides the utility as usual. The definitions
in the matching header of the library have been moved into the
v8::interpreter::internal namespace.
The library exposes a class ExpectationPrinter, with a method
PrintExpectation, which takes a test snippet as input, and writes the
formatted expectation to the supplied stream. One might then use a
std::stringstream to retrieve the results as a string and run it through
a textual diff utility.
BUG=v8:4280
LOG=N
==========
Description was changed from
==========
[Interpreter] Change the output format of generate-bytecode-expectations.
Now the tool produces a far more readable output format, which bears a
lot of resemblance to YAML. In fact, the output should be machine
parseable as such, one document per testcase. However, the output format
may be subject to changes in future, so don't rely on this property.
In general, the output format has been optimized for meaningful diffing,
while keeping a decent readability, so not everything is as compact as
it could be, e.g. for an empty const pool we get:
constant pool: [
]
instead of:
constant pool: []
Also, trailing commas are always inserted in lists.
Additionally, now the tool accepts its output format as input. When
operating in this mode, all the snippets are extracted, processed and
the output is then emitted as usual. If nothing has changed, the output
should match the input. This is very useful for catching bugs in the
bytecode generation by running a textual diff against a known-good file.
The core (namely bytecode-expectations.cc) has been extracted from the
original cc file, which provides the utility as usual. The definitions
in the matching header of the library have been moved into the
v8::interpreter::internal namespace.
The library exposes a class ExpectationPrinter, with a method
PrintExpectation, which takes a test snippet as input, and writes the
formatted expectation to the supplied stream. One might then use a
std::stringstream to retrieve the results as a string and run it through
a textual diff utility.
BUG=v8:4280
LOG=N
==========
to
==========
[Interpreter] Change the output format of generate-bytecode-expectations.
Now the tool produces a far more readable output format, which bears a
lot of resemblance to YAML. In fact, the output should be machine
parseable as such, one document per testcase. However, the output format
may be subject to changes in future, so don't rely on this property.
In general, the output format has been optimized for producing a meaningful
textual diff, while keeping a decent readability as well. Therefore, not
everything is as compact as it could be, e.g. for an empty const pool we get:
constant pool: [
]
instead of:
constant pool: []
Also, trailing commas are always inserted in lists.
Additionally, now the tool accepts its output format as input. When
operating in this mode, all the snippets are extracted, processed and
the output is then emitted as usual. If nothing has changed, the output
should match the input. This is very useful for catching bugs in the
bytecode generation by running a textual diff against a known-good file.
The core (namely bytecode-expectations.cc) has been extracted from the
original cc file, which provides the utility as usual. The definitions
in the matching header of the library have been moved into the
v8::interpreter::internal namespace.
The library exposes a class ExpectationPrinter, with a method
PrintExpectation, which takes a test snippet as input, and writes the
formatted expectation to the supplied stream. One might then use a
std::stringstream to retrieve the results as a string and run it through
a diff utility.
BUG=v8:4280
LOG=N
==========
Description was changed from
==========
[Interpreter] Change the output format of generate-bytecode-expectations.
Now the tool produces a far more readable output format, which bears a
lot of resemblance to YAML. In fact, the output should be machine
parseable as such, one document per testcase. However, the output format
may be subject to changes in future, so don't rely on this property.
In general, the output format has been optimized for producing a meaningful
textual diff, while keeping a decent readability as well. Therefore, not
everything is as compact as it could be, e.g. for an empty const pool we get:
constant pool: [
]
instead of:
constant pool: []
Also, trailing commas are always inserted in lists.
Additionally, now the tool accepts its output format as input. When
operating in this mode, all the snippets are extracted, processed and
the output is then emitted as usual. If nothing has changed, the output
should match the input. This is very useful for catching bugs in the
bytecode generation by running a textual diff against a known-good file.
The core (namely bytecode-expectations.cc) has been extracted from the
original cc file, which provides the utility as usual. The definitions
in the matching header of the library have been moved into the
v8::interpreter::internal namespace.
The library exposes a class ExpectationPrinter, with a method
PrintExpectation, which takes a test snippet as input, and writes the
formatted expectation to the supplied stream. One might then use a
std::stringstream to retrieve the results as a string and run it through
a diff utility.
BUG=v8:4280
LOG=N
==========
to
==========
[Interpreter] Change the output format of generate-bytecode-expectations.
Now the tool produces a far more readable output format, which bears a
lot of resemblance to YAML. In fact, the output should be machine
parseable as such, one document per testcase. However, the output format
may be subject to changes in future, so don't rely on this property.
In general, the output format has been optimized for producing a meaningful
textual diff, while keeping a decent readability as well. Therefore, not
everything is as compact as it could be, e.g. for an empty const pool we get:
constant pool: [
]
instead of:
constant pool: []
Also, trailing commas are always inserted in lists.
Additionally, now the tool accepts its output format as input. When
operating in this mode, all the snippets are extracted, processed and
the output is then emitted as usual. If nothing has changed, the output
should match the input. This is very useful for catching bugs in the
bytecode generation by running a textual diff against a known-good file.
The core (namely bytecode-expectations.cc) has been extracted from the
original cc file, which provides the utility as usual. The definitions
in the matching header of the library have been moved into the
v8::internal::interpreter namespace.
The library exposes a class ExpectationPrinter, with a method
PrintExpectation, which takes a test snippet as input, and writes the
formatted expectation to the supplied stream. One might then use a
std::stringstream to retrieve the results as a string and run it through
a diff utility.
BUG=v8:4280
LOG=N
==========
The CQ bit was checked by ssanfilippo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688383003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688383003/240001
Message was sent while issue was closed.
Description was changed from
==========
[Interpreter] Change the output format of generate-bytecode-expectations.
Now the tool produces a far more readable output format, which bears a
lot of resemblance to YAML. In fact, the output should be machine
parseable as such, one document per testcase. However, the output format
may be subject to changes in future, so don't rely on this property.
In general, the output format has been optimized for producing a meaningful
textual diff, while keeping a decent readability as well. Therefore, not
everything is as compact as it could be, e.g. for an empty const pool we get:
constant pool: [
]
instead of:
constant pool: []
Also, trailing commas are always inserted in lists.
Additionally, now the tool accepts its output format as input. When
operating in this mode, all the snippets are extracted, processed and
the output is then emitted as usual. If nothing has changed, the output
should match the input. This is very useful for catching bugs in the
bytecode generation by running a textual diff against a known-good file.
The core (namely bytecode-expectations.cc) has been extracted from the
original cc file, which provides the utility as usual. The definitions
in the matching header of the library have been moved into the
v8::internal::interpreter namespace.
The library exposes a class ExpectationPrinter, with a method
PrintExpectation, which takes a test snippet as input, and writes the
formatted expectation to the supplied stream. One might then use a
std::stringstream to retrieve the results as a string and run it through
a diff utility.
BUG=v8:4280
LOG=N
==========
to
==========
[Interpreter] Change the output format of generate-bytecode-expectations.
Now the tool produces a far more readable output format, which bears a
lot of resemblance to YAML. In fact, the output should be machine
parseable as such, one document per testcase. However, the output format
may be subject to changes in future, so don't rely on this property.
In general, the output format has been optimized for producing a meaningful
textual diff, while keeping a decent readability as well. Therefore, not
everything is as compact as it could be, e.g. for an empty const pool we get:
constant pool: [
]
instead of:
constant pool: []
Also, trailing commas are always inserted in lists.
Additionally, now the tool accepts its output format as input. When
operating in this mode, all the snippets are extracted, processed and
the output is then emitted as usual. If nothing has changed, the output
should match the input. This is very useful for catching bugs in the
bytecode generation by running a textual diff against a known-good file.
The core (namely bytecode-expectations.cc) has been extracted from the
original cc file, which provides the utility as usual. The definitions
in the matching header of the library have been moved into the
v8::internal::interpreter namespace.
The library exposes a class ExpectationPrinter, with a method
PrintExpectation, which takes a test snippet as input, and writes the
formatted expectation to the supplied stream. One might then use a
std::stringstream to retrieve the results as a string and run it through
a diff utility.
BUG=v8:4280
LOG=N
==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Description was changed from
==========
[Interpreter] Change the output format of generate-bytecode-expectations.
Now the tool produces a far more readable output format, which bears a
lot of resemblance to YAML. In fact, the output should be machine
parseable as such, one document per testcase. However, the output format
may be subject to changes in future, so don't rely on this property.
In general, the output format has been optimized for producing a meaningful
textual diff, while keeping a decent readability as well. Therefore, not
everything is as compact as it could be, e.g. for an empty const pool we get:
constant pool: [
]
instead of:
constant pool: []
Also, trailing commas are always inserted in lists.
Additionally, now the tool accepts its output format as input. When
operating in this mode, all the snippets are extracted, processed and
the output is then emitted as usual. If nothing has changed, the output
should match the input. This is very useful for catching bugs in the
bytecode generation by running a textual diff against a known-good file.
The core (namely bytecode-expectations.cc) has been extracted from the
original cc file, which provides the utility as usual. The definitions
in the matching header of the library have been moved into the
v8::internal::interpreter namespace.
The library exposes a class ExpectationPrinter, with a method
PrintExpectation, which takes a test snippet as input, and writes the
formatted expectation to the supplied stream. One might then use a
std::stringstream to retrieve the results as a string and run it through
a diff utility.
BUG=v8:4280
LOG=N
==========
to
==========
[Interpreter] Change the output format of generate-bytecode-expectations.
Now the tool produces a far more readable output format, which bears a
lot of resemblance to YAML. In fact, the output should be machine
parseable as such, one document per testcase. However, the output format
may be subject to changes in future, so don't rely on this property.
In general, the output format has been optimized for producing a meaningful
textual diff, while keeping a decent readability as well. Therefore, not
everything is as compact as it could be, e.g. for an empty const pool we get:
constant pool: [
]
instead of:
constant pool: []
Also, trailing commas are always inserted in lists.
Additionally, now the tool accepts its output format as input. When
operating in this mode, all the snippets are extracted, processed and
the output is then emitted as usual. If nothing has changed, the output
should match the input. This is very useful for catching bugs in the
bytecode generation by running a textual diff against a known-good file.
The core (namely bytecode-expectations.cc) has been extracted from the
original cc file, which provides the utility as usual. The definitions
in the matching header of the library have been moved into the
v8::internal::interpreter namespace.
The library exposes a class ExpectationPrinter, with a method
PrintExpectation, which takes a test snippet as input, and writes the
formatted expectation to the supplied stream. One might then use a
std::stringstream to retrieve the results as a string and run it through
a diff utility.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/e082ebdbf3f352d55850c87dff3d5d01d772655b
Cr-Commit-Position: refs/heads/master@{#33997}
==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/e082ebdbf3f352d55850c87dff3d5d01d772655b Cr-Commit-Position: refs/heads/master@{#33997} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
