[Interpreter] generate-bytecode-expectations improvements.

test/cctest/interpreter/generate-bytecode-expectations.cc

Index: test/cctest/interpreter/generate-bytecode-expectations.cc
diff --git a/test/cctest/interpreter/generate-bytecode-expectations.cc b/test/cctest/interpreter/generate-bytecode-expectations.cc
index 8bf9ba6dfb9ac28e459e71bf1f7480c1962bd074..8b13dcf812a3e16ae872fa17243c689d48cb82e5 100644
--- a/test/cctest/interpreter/generate-bytecode-expectations.cc
+++ b/test/cctest/interpreter/generate-bytecode-expectations.cc
@@ -28,6 +28,10 @@ class ProgramOptions {
         print_help_(false),
         read_raw_js_snippet_(false),
         read_from_stdin_(false),
+        fix_(false),
+        print_banner_(true),
+        wrap_(true),
+        execute_(true),
         const_pool_type_(
             BytecodeExpectationsPrinter::ConstantPoolType::kMixed) {}
 
@@ -37,18 +41,32 @@ class ProgramOptions {
   bool print_help() const { return print_help_; }
   bool read_raw_js_snippet() const { return read_raw_js_snippet_; }
   bool read_from_stdin() const { return read_from_stdin_; }
-  std::string filename() const { return filename_; }
+  bool fix() const { return fix_; }
+  bool print_banner() const { return print_banner_; }
+  bool wrap() const { return wrap_; }
+  bool execute() const { return execute_; }
   BytecodeExpectationsPrinter::ConstantPoolType const_pool_type() const {
     return const_pool_type_;
   }
+  std::string input_filename() const { return input_filename_; }
+  std::string output_filename() const { return output_file_name_; }
+  std::string top_function_name() const { return top_function_name_; }
 
  private:
   bool parsing_failed_;
   bool print_help_;
   bool read_raw_js_snippet_;
   bool read_from_stdin_;
+  bool fix_;
+  bool print_banner_;
+  bool wrap_;
+  bool execute_;
   BytecodeExpectationsPrinter::ConstantPoolType const_pool_type_;
-  std::string filename_;
+  std::string input_filename_;
+  std::string output_file_name_;
+  std::string top_function_name_;
+
+  friend void ParseOptionsHeader(ProgramOptions*, std::istream&);
 };
 
 class ArrayBufferAllocator final : public v8::ArrayBuffer::Allocator {
@@ -95,8 +113,6 @@ BytecodeExpectationsPrinter::ConstantPoolType ParseConstantPoolType(
 ProgramOptions ProgramOptions::FromCommandLine(int argc, char** argv) {
   ProgramOptions options;
 
-  if (argc <= 1) return options;
-
   for (int i = 1; i < argc; ++i) {
     if (strcmp(argv[i], "--help") == 0) {
       options.print_help_ = true;
@@ -106,13 +122,25 @@ ProgramOptions ProgramOptions::FromCommandLine(int argc, char** argv) {
       options.const_pool_type_ = ParseConstantPoolType(argv[i] + 12);
     } else if (strcmp(argv[i], "--stdin") == 0) {
       options.read_from_stdin_ = true;
+    } else if (strcmp(argv[i], "--fix") == 0) {
+      options.fix_ = true;
+    } else if (strcmp(argv[i], "--no-banner") == 0) {
+      options.print_banner_ = false;
+    } else if (strcmp(argv[i], "--no-wrap") == 0) {
+      options.wrap_ = false;
+    } else if (strcmp(argv[i], "--no-execute") == 0) {
+      options.execute_ = false;
+    } else if (strncmp(argv[i], "--output=", 9) == 0) {
+      options.output_file_name_ = argv[i] + 9;
+    } else if (strncmp(argv[i], "--wrapper-name=", 15) == 0) {
+      options.top_function_name_ = argv[i] + 15;
     } else if (strncmp(argv[i], "--", 2) != 0) {  // It doesn't start with --
-      if (!options.filename_.empty()) {
+      if (!options.input_filename_.empty()) {
         std::cerr << "ERROR: More than one input file specified\n";
         options.parsing_failed_ = true;
         break;
       }
-      options.filename_ = argv[i];
+      options.input_filename_ = argv[i];
     } else {
       std::cerr << "ERROR: Unknonwn option " << argv[i] << "\n";
       options.parsing_failed_ = true;
@@ -123,6 +151,75 @@ ProgramOptions ProgramOptions::FromCommandLine(int argc, char** argv) {
   return options;
 }
 
+bool ParseBoolean(const char* string) {
+  if (strcmp(string, "yes") == 0) {
+    return true;
+  } else if (strcmp(string, "no") == 0) {
+    return false;
+  } else {
+    UNREACHABLE();
+    return false;
+  }
+}
+
+void ParseOptionsHeader(ProgramOptions* options,
+                        std::istream& stream) {  // NOLINT
+  std::string line;
+
+  // Skip to the beginning of the options header
+  while (std::getline(stream, line)) {
+    if (line == "---") break;
+  }
+
+  while (std::getline(stream, line)) {
+    if (line.compare(0, 11, "pool type: ") == 0) {
+      options->const_pool_type_ = ParseConstantPoolType(line.c_str() + 11);
+    } else if (line.compare(0, 9, "execute: ") == 0) {
+      options->execute_ = ParseBoolean(line.c_str() + 9);
+    } else if (line.compare(0, 6, "wrap: ") == 0) {
+      options->wrap_ = ParseBoolean(line.c_str() + 6);
+    } else if (line.compare(0, 13, "wrapper name: ") == 0) {
+      options->top_function_name_ = line.c_str() + 14;
+    } else {
+      break;
+    }
+  }
+}
+
+const char* BooleanString(bool value) { return value ? "yes" : "no"; }

[rmcilroy, 2016/02/16 16:43:48]

nit - move up near ParseBoolean

[Stefano Sanfilippo, 2016/02/16 20:28:26]

Done.

+
+const char* ConstantPoolTypeString(
+    BytecodeExpectationsPrinter::ConstantPoolType type) {
+  switch (type) {
+    case BytecodeExpectationsPrinter::ConstantPoolType::kDouble:
+      return "double";
+    case BytecodeExpectationsPrinter::ConstantPoolType::kInteger:
+      return "integer";
+    case BytecodeExpectationsPrinter::ConstantPoolType::kMixed:
+      return "mixed";
+    case BytecodeExpectationsPrinter::ConstantPoolType::kString:
+      return "string";
+    default:
+      UNREACHABLE();
+      return nullptr;
+  }
+}
+
+void EmitOptionsHeader(std::ostream& stream,  // NOLINT
+                       const ProgramOptions& options) {
+  stream << "---"
+            "\npool type: "
+         << ConstantPoolTypeString(options.const_pool_type())
+         << "\nexecute: " << BooleanString(options.execute())
+         << "\nwrap: " << BooleanString(options.wrap());
+
+  if (!options.top_function_name().empty()) {
+    stream << "\nwrapper name: " << options.top_function_name();
+  }

[rmcilroy, 2016/02/16 16:43:48]

Could you emit a seperator between the options and the first snippet too (e.g.,
another "---") and use it to end the loop in ParseOptionsHeader

[Stefano Sanfilippo, 2016/02/16 20:28:26]

I have updated the code to use the separator of the next block (namely, the
first expectation) to end the parsing of the header. Is it OK?

Also, I've made the parser a bit more robust by rejecting any line which is
neither a known configuration option nor a blank one.

+
+  stream << "\n\n";
+}
+
 bool ProgramOptions::Validate() const {
   if (parsing_failed_) return false;
   if (print_help_) return true;
@@ -133,16 +230,21 @@ bool ProgramOptions::Validate() const {
     return false;
   }
 
-  if (!read_from_stdin_ && filename_.empty()) {
+  if (!read_from_stdin_ && input_filename_.empty()) {
     std::cerr << "ERROR: No input file specified.\n";
     return false;
   }
 
-  if (read_from_stdin_ && !filename_.empty()) {
+  if (read_from_stdin_ && !input_filename_.empty()) {
     std::cerr << "ERROR: Reading from stdin, but input files supplied.\n";
     return false;
   }
 
+  if (!wrap_ && !top_function_name_.empty()) {
+    std::cerr << "ERROR: not wrapping, but wrapper name specified.\n";
+    return false;
+  }
+

[rmcilroy, 2016/02/16 16:43:48]

You should also validate that --rebaseline and --raw-js aren't specified
together.

[Stefano Sanfilippo, 2016/02/16 20:28:26]

Done.

   return true;
 }
 
@@ -194,41 +296,64 @@ bool ReadNextSnippet(std::istream& stream, std::string* string_out) {  // NOLINT
   return false;
 }
 
+std::string UnescapeString(const std::string& escaped_string) {
+  std::string unescaped_string;
+  bool previous_was_backslash = false;
+  for (char c : escaped_string) {
+    if (previous_was_backslash) {
+      // If it was not an escape sequence, emit the backslash

[rmcilroy, 2016/02/16 16:43:48]

/s/the backslash/the previous backslash./

[Stefano Sanfilippo, 2016/02/16 20:28:26]

Done.

+      if (c != '\\' && c != '"') unescaped_string += '\\';
+      unescaped_string += c;
+      previous_was_backslash = false;
+    } else {
+      if (c == '\\') {
+        previous_was_backslash = true;
+        // Defer emission to the point where we can check if it was an escape.
+      } else {
+        unescaped_string += c;
+      }
+    }
+  }
+  return unescaped_string;
+}
+
 void ExtractSnippetsFromStream(std::vector<std::string>* snippet_list,
+                               ProgramOptions* options,
                                std::istream& body_stream,  // NOLINT
                                bool read_raw_js_snippet) {
   if (read_raw_js_snippet) {
     snippet_list->push_back(ReadRawJSSnippet(body_stream));
   } else {
+    ParseOptionsHeader(options, body_stream);

[rmcilroy, 2016/02/16 16:43:48]

This should only happen for --rebaseline I think (and should probably be moved
out of ExtractSnippetsFromStream and instead called directly from main (i.e.,
immediately after the are read from the command line).

[Stefano Sanfilippo, 2016/02/16 20:28:26]

Please see below.

     std::string snippet;
     while (ReadNextSnippet(body_stream, &snippet)) {
-      snippet_list->push_back(snippet);
+      snippet_list->push_back(UnescapeString(snippet));
     }
   }
 }
 
 bool ExtractSnippets(std::vector<std::string>* snippet_list,
-                     const ProgramOptions& options) {
-  if (options.read_from_stdin()) {
-    ExtractSnippetsFromStream(snippet_list, std::cin,
-                              options.read_raw_js_snippet());
+                     ProgramOptions* options) {

[rmcilroy, 2016/02/16 16:43:48]

This should still be const, right? Any reason to move to a pointer rather than
reference?

[Stefano Sanfilippo, 2016/02/16 20:28:26]

The reason is that ParseOptionsHeader called inside ExtractSnippetsFromStream
can and will modify ProgramOptions. I realise it's not super straightforward, so
I'm trying to move the ParseOptionsHeader call as close to the main in the call
graph as possible.

Unfortunately we can't call it directly in the main, because we need to
determine and prepare the input stream (either stdin or file), which is exactly
the task performed by ExtractSnippets before calling ExtractSnippetsFromStream.

I have moved the incriminated call to ExtraxtSnippets and I have also renamed it
to MaybeUpdateOptionsFromHeader in order to make it clearer that program options
might (or might not) be modified.

I am open to other solutions, though.

[rmcilroy, 2016/02/17 10:16:59]

I think the solution is to split out the opening of the file, creation of the
stream and then reading of the header as seperate functions from ExtractSnippets
- with ExtractSnippets just taking the already open stream, WDYT?

[rmcilroy, 2016/02/17 10:16:59]

I think the solution is to split out the opening of the file, creation of the
stream and then reading of the header as seperate functions from ExtractSnippets
- with ExtractSnippets just taking the already open stream, WDYT?

[Stefano Sanfilippo, 2016/02/17 14:32:29]

I've rewritten the piece using a better approach which allows us to move
MaybeUpdateOptionsFromHeader in the main(). Not as compact as I wish it could
be, but still better than before. WDYT?

+  if (options->read_from_stdin()) {
+    ExtractSnippetsFromStream(snippet_list, options, std::cin,
+                              options->read_raw_js_snippet());
   } else {
-    std::ifstream body_file(options.filename().c_str());
+    std::ifstream body_file(options->input_filename().c_str());
     if (!body_file.is_open()) {
-      std::cerr << "ERROR: Could not open '" << options.filename() << "'.\n";
+      std::cerr << "ERROR: Could not open '" << options->input_filename()
+                << "'.\n";
       return false;
     }
-    ExtractSnippetsFromStream(snippet_list, body_file,
-                              options.read_raw_js_snippet());
+    ExtractSnippetsFromStream(snippet_list, options, body_file,
+                              options->read_raw_js_snippet());
   }
   return true;
 }
 
-void GenerateExpectationsFile(
-    std::ostream& stream,  // NOLINT
-    const std::vector<std::string>& snippet_list,
-    BytecodeExpectationsPrinter::ConstantPoolType const_pool_type,
-    const char* exec_path) {
+void GenerateExpectationsFile(std::ostream& stream,  // NOLINT
+                              const std::vector<std::string>& snippet_list,
+                              const ProgramOptions& options,
+                              const char* exec_path) {
   V8InitializationScope platform(exec_path);
   {
     v8::Isolate::Scope isolate_scope(platform.isolate());
@@ -236,9 +361,20 @@ void GenerateExpectationsFile(
     v8::Local<v8::Context> context = v8::Context::New(platform.isolate());
     v8::Context::Scope context_scope(context);
 
-    stream << "#\n# Autogenerated by generate-bytecode-expectations\n#\n\n";
+    if (options.print_banner()) {
+      stream << "#\n# Autogenerated by generate-bytecode-expectations\n#\n\n";
+    }
+
+    EmitOptionsHeader(stream, options);
+
+    BytecodeExpectationsPrinter printer(platform.isolate(),
+                                        options.const_pool_type());
+    printer.set_wrap(options.wrap());
+    printer.set_execute(options.execute());
+    if (!options.top_function_name().empty()) {
+      printer.set_top_function_name(options.top_function_name());
+    }
 
-    BytecodeExpectationsPrinter printer(platform.isolate(), const_pool_type);
     for (const std::string& snippet : snippet_list) {
       printer.PrintExpectation(stream, snippet);
     }
@@ -253,8 +389,15 @@ void PrintUsage(const char* exec_path) {
          "  --help    Print this help message.\n"
          "  --raw-js  Read raw JavaScript, instead of the output format.\n"
          "  --stdin   Read from standard input instead of file.\n"
+         "  --fix     Overwrite input file.\n"

[rmcilroy, 2016/02/16 16:43:48]

Call this "--rebaseline  Rebaseline input snippet file"

[Stefano Sanfilippo, 2016/02/16 20:28:26]

Done.

+         "  --no-banner   Do not add the \"generated by\" banner.\n"

[rmcilroy, 2016/02/16 16:43:48]

Why is this one required? All golden files should have this banner shouldn't
they?

[Stefano Sanfilippo, 2016/02/16 20:28:26]

ATM the banner is emitted by the utility before calling the library, because
PrintExpectation is called with one snippet at a time, so we would end up
repeating the header before each snippet if the call was moved there. I am still
thinking about the best way of emitting the header while using the library
outside the utility. Probably something like PrintHeader with an internal
consistency check in PrintExpectation (e.g. CHECK(header_emitted_)). This is a
temporary solution, but IMHO it might as well remain as it's not adding
complexity to the code.

[rmcilroy, 2016/02/17 10:17:00]

I think the banner should be printed by this tool once at the top of the file
(i.e., before the program options), that way you can remove this from the flags
passed to BytecodeExpectationPrinter.

[Stefano Sanfilippo, 2016/02/17 14:32:29]

Done. I ended up changing the API from accepting a single snippet per call to
taking an std::vector of snippets.

+         "  --no-wrap     Do not wrap the snippet in a function.\n"
+         "  --no-execute  Do not execute after compilation.\n"
+         "  --output=file.name  Specify the output file. "
+         "If not specified, output goes to stdout.\n"
+         "  --wrapper-name=foo  Specify the name of the wrapper function.\n"
          "  --pool-type=(int|double|string|mixed)\n"
-         "      specify the type of the entries in the constant pool "
+         "      Specify the type of the entries in the constant pool "
          "(default: mixed).\n"
          "\n"
          "Each raw JavaScript file is interpreted as a single snippet.\n\n"
@@ -274,10 +417,23 @@ int main(int argc, char** argv) {
   }
 
   std::vector<std::string> snippet_list;
-  if (!ExtractSnippets(&snippet_list, options)) {
+  if (!ExtractSnippets(&snippet_list, &options)) {
     return 2;
   }
 
-  GenerateExpectationsFile(std::cout, snippet_list, options.const_pool_type(),
-                           argv[0]);
+  CHECK(options.Validate());
+
+  if (!options.output_filename().empty() || options.fix()) {
+    std::ofstream output_file(options.fix()
+                                  ? options.input_filename().c_str()
+                                  : options.output_filename().c_str());
+    if (!output_file.is_open()) {
+      std::cerr << "ERROR: Could not open '" << options.output_filename()
+                << "' for writing.\n";
+      return 3;
+    }
+    GenerateExpectationsFile(output_file, snippet_list, options, argv[0]);
+  } else {
+    GenerateExpectationsFile(std::cout, snippet_list, options, argv[0]);
+  }
 }