[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

courgette/courgette_tool.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <string>
 #include <vector>
 
 #include "base/at_exit.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "courgette/assembly_program.h"
 #include "courgette/courgette.h"
+#include "courgette/encoded_program.h"
+#include "courgette/program_detector.h"
 #include "courgette/streams.h"
 #include "courgette/third_party/bsdiff.h"
 
 
 void PrintHelp() {
   fprintf(stderr,
     "Usage:\n"
     "  courgette -supported <executable_file>\n"
     "  courgette -dis <executable_file> <binary_assembly_file>\n"
     "  courgette -asm <binary_assembly_file> <executable_file>\n"
@@ skipping 39 matching lines @@
   if (count == -1)
     Problem("Can't write output.");
   if (static_cast<size_t>(count) != sink->Length())
     Problem("Incomplete write.");
 }
 
 void Disassemble(const base::FilePath& input_file,
                  const base::FilePath& output_file) {
   std::string buffer = ReadOrFail(input_file, "input");
 
-  courgette::AssemblyProgram* program = NULL;
+  scoped_ptr<courgette::AssemblyProgram> program;

[huangs, 2016/01/25 19:22:48]

Style: I prefer to put error checking immediately after the call being checked. 
Also, explicit deallocation (e.g., program.reset(nullptr);) is memory
deallocation that isn't directly related to main logic, and should stand out in
a separate block.

   const courgette::Status parse_status =
       courgette::ParseDetectedExecutable(buffer.c_str(), buffer.length(),
                                          &program);
-
   if (parse_status != courgette::C_OK)
     Problem("Can't parse input (code = %d).", parse_status);
 
-  courgette::EncodedProgram* encoded = NULL;
-  const courgette::Status encode_status = Encode(program, &encoded);
-
-  courgette::DeleteAssemblyProgram(program);
-
+  scoped_ptr<courgette::EncodedProgram> encoded;

[grt (UTC plus 2), 2016/01/26 16:39:54]

yeah, i'm not sure why this shouldn't be:
  scoped_ptr<courgette::EncodedProgram> encoded = program->Encode();
  if (!encoded)
    Problem("Can't encode program.");

oh, wait, i get it. courgette.h defines a simple C API where the allocation
boundary is well-defined (all allocations within the impl have corresponding
deallocations within the impl). i think your change is bigger than you intend,
as it moves deallocation out to the caller. this is problematic since the
caller's impl of "delete" may not be the same as the impl's.  in practice, it
looks like this is safe since DeleteEncodedProgram just calls "delete", and
courgette is a static lib, so it should be bound to the proper impl even if the
consumer of the API calls delete.

[Will Harris, 2016/01/26 18:40:20]

Just to comment on this point, courgette doesn't expose any exports it's only
exposed as a c++ header, so callers to it will always be in the same process
space so should use the same delete/new impl. This change should therefore, in
general, be safe as regards to that... I think? Can you point to a specific case
where you think a caller might use a different delete/new?

more review to come.

[huangs, 2016/01/26 18:42:24]

Excellent point!  Two issues of debate are:

A. Allocation boundaries: Courette does file-mapped I/O, but that's done through
std::vector-like NoThrowBuffer, not by operators new/delete (operator new does
get overloaded in memory_monitor.cc, but that code looks unused?).  Courgette
also uses regular new and delete all over.  So maybe there's no boundary as far
as new/delete is concerned, and hence this CL is not that disruptive??

B. The Courgette API: The key interface to rest of Chrome seems to be
- courgette::ApplyBinaryPatch() for BSDiff,
- courgette::ApplyEnsemblePatch() for Courgette,
and nobody seems to care about AssemblyProgram details and such (which is good:
I think AssemblyProgram & EncodedProgram can be merged).  So the main user of
the "exposed API" in courgette.h is just courgette_tool.cc.  I'd argue that
AssemblyProgram and EncodedProgram should not be part of the API.

So I prefer leaving things as-is.  BUT suppose there is an allocation boundary
(A) that we should respect, and that we want to expose AssemblyProgram &
EncodedProgram (B), then here are some alternatives:

1. Create CourgetteDeleter, use scoped_ptr<T, CourgetteDeleter> throughout.
2. Make AssemblyProgram and EncodedProgram "consumed after use", which fits the
existing usage pattern.  We can supply Clone() if ever need.
3. Add AssemblyProgramManager and EncodedProgramManager to manage
AssemblyProgram and EncodedProgram the Courgette way behind the scenes (like
std::string).  We can also add Dispose() to enable early deallocation to save
memory.

+  const courgette::Status encode_status = Encode(*program, &encoded);
   if (encode_status != courgette::C_OK)
     Problem("Can't encode program.");
 
+  program.reset(nullptr);

[grt (UTC plus 2), 2016/01/26 16:39:54]

reset() here and everywhere

[huangs, 2016/01/26 18:42:24]

Done.

+
   courgette::SinkStreamSet sinks;
-
   const courgette::Status write_status =
-    courgette::WriteEncodedProgram(encoded, &sinks);
+    courgette::WriteEncodedProgram(encoded.get(), &sinks);
   if (write_status != courgette::C_OK)
     Problem("Can't serialize encoded program.");
 
-  courgette::DeleteEncodedProgram(encoded);
+  encoded.reset(nullptr);
 
   courgette::SinkStream sink;
   if (!sinks.CopyTo(&sink))
     Problem("Can't combine serialized encoded program streams.");
 
   WriteSinkToFile(&sink, output_file);
 }
 
 bool Supported(const base::FilePath& input_file) {
   bool result = false;
@@ skipping 39 matching lines @@
   printf("%s Executable\n", format.c_str());
   return result;
 }
 
 void DisassembleAndAdjust(const base::FilePath& program_file,
                           const base::FilePath& model_file,
                           const base::FilePath& output_file) {
   std::string program_buffer = ReadOrFail(program_file, "program");
   std::string model_buffer = ReadOrFail(model_file, "reference");
 
-  courgette::AssemblyProgram* program = NULL;
+  scoped_ptr<courgette::AssemblyProgram> program;
   const courgette::Status parse_program_status =
       courgette::ParseDetectedExecutable(program_buffer.c_str(),
                                          program_buffer.length(),
                                          &program);
   if (parse_program_status != courgette::C_OK)
     Problem("Can't parse program input (code = %d).", parse_program_status);
 
-  courgette::AssemblyProgram* model = NULL;
+  scoped_ptr<courgette::AssemblyProgram> model;
   const courgette::Status parse_model_status =
       courgette::ParseDetectedExecutable(model_buffer.c_str(),
                                          model_buffer.length(),
                                          &model);
   if (parse_model_status != courgette::C_OK)
     Problem("Can't parse model input (code = %d).", parse_model_status);
 
-  const courgette::Status adjust_status = Adjust(*model, program);
+  const courgette::Status adjust_status = Adjust(*model, program.get());
   if (adjust_status != courgette::C_OK)
     Problem("Can't adjust program.");
 
-  courgette::EncodedProgram* encoded = NULL;
-  const courgette::Status encode_status = Encode(program, &encoded);
+  model.reset(nullptr);
 
-  courgette::DeleteAssemblyProgram(program);
-
+  scoped_ptr<courgette::EncodedProgram> encoded;
+  const courgette::Status encode_status = Encode(*program, &encoded);
   if (encode_status != courgette::C_OK)
     Problem("Can't encode program.");
 
+  program.reset(nullptr);
+
   courgette::SinkStreamSet sinks;
-
   const courgette::Status write_status =
-    courgette::WriteEncodedProgram(encoded, &sinks);
+    courgette::WriteEncodedProgram(encoded.get(), &sinks);
   if (write_status != courgette::C_OK)
     Problem("Can't serialize encoded program.");
 
-  courgette::DeleteEncodedProgram(encoded);
+  encoded.reset(nullptr);
 
   courgette::SinkStream sink;
   if (!sinks.CopyTo(&sink))
     Problem("Can't combine serialized encoded program streams.");
 
   WriteSinkToFile(&sink, output_file);
 }
 
 // Diffs two executable files, write a set of files for the diff, one file per
 // stream of the EncodedProgram format.  Each file is the bsdiff between the
 // original file's stream and the new file's stream.  This is completely
 // uninteresting to users, but it is handy for seeing how much each which
 // streams are contributing to the final file size.  Adjustment is optional.
 void DisassembleAdjustDiff(const base::FilePath& model_file,
                            const base::FilePath& program_file,
                            const base::FilePath& output_file_root,
                            bool adjust) {
   std::string model_buffer = ReadOrFail(model_file, "'old'");
   std::string program_buffer = ReadOrFail(program_file, "'new'");
 
-  courgette::AssemblyProgram* model = NULL;
+  scoped_ptr<courgette::AssemblyProgram> model;
   const courgette::Status parse_model_status =
       courgette::ParseDetectedExecutable(model_buffer.c_str(),
                                          model_buffer.length(),
                                          &model);
   if (parse_model_status != courgette::C_OK)
     Problem("Can't parse model input (code = %d).", parse_model_status);
 
-  courgette::AssemblyProgram* program = NULL;
+  scoped_ptr<courgette::AssemblyProgram> program;
   const courgette::Status parse_program_status =
       courgette::ParseDetectedExecutable(program_buffer.c_str(),
                                          program_buffer.length(),
                                          &program);
   if (parse_program_status != courgette::C_OK)
     Problem("Can't parse program input (code = %d).", parse_program_status);
 
   if (adjust) {
-    const courgette::Status adjust_status = Adjust(*model, program);
+    const courgette::Status adjust_status = Adjust(*model, program.get());
     if (adjust_status != courgette::C_OK)
       Problem("Can't adjust program.");
   }
 
-  courgette::EncodedProgram* encoded_program = NULL;
+  scoped_ptr<courgette::EncodedProgram> encoded_program;
   const courgette::Status encode_program_status =
-      Encode(program, &encoded_program);
-  courgette::DeleteAssemblyProgram(program);
+      Encode(*program, &encoded_program);
   if (encode_program_status != courgette::C_OK)
     Problem("Can't encode program.");
 
-  courgette::EncodedProgram* encoded_model = NULL;
-  const courgette::Status encode_model_status = Encode(model, &encoded_model);
-  courgette::DeleteAssemblyProgram(model);
+  program.reset(nullptr);
+
+  scoped_ptr<courgette::EncodedProgram> encoded_model;
+  const courgette::Status encode_model_status = Encode(*model, &encoded_model);
   if (encode_model_status != courgette::C_OK)
     Problem("Can't encode model.");
 
+  model.reset(nullptr);
+
   courgette::SinkStreamSet program_sinks;
   const courgette::Status write_program_status =
-    courgette::WriteEncodedProgram(encoded_program, &program_sinks);
+    courgette::WriteEncodedProgram(encoded_program.get(), &program_sinks);
   if (write_program_status != courgette::C_OK)
     Problem("Can't serialize encoded program.");
-  courgette::DeleteEncodedProgram(encoded_program);
+
+  encoded_program.reset();
 
   courgette::SinkStreamSet model_sinks;
   const courgette::Status write_model_status =
-    courgette::WriteEncodedProgram(encoded_model, &model_sinks);
+    courgette::WriteEncodedProgram(encoded_model.get(), &model_sinks);
   if (write_model_status != courgette::C_OK)
     Problem("Can't serialize encoded model.");
-  courgette::DeleteEncodedProgram(encoded_model);
+
+  encoded_model.reset(nullptr);
 
   courgette::SinkStream empty_sink;
   for (int i = 0;  ; ++i) {
     courgette::SinkStream* old_stream = model_sinks.stream(i);
     courgette::SinkStream* new_stream = program_sinks.stream(i);
     if (old_stream == NULL && new_stream == NULL)
       break;
 
     courgette::SourceStream old_source;
     courgette::SourceStream new_source;
@@ skipping 12 matching lines @@
 }
 
 void Assemble(const base::FilePath& input_file,
               const base::FilePath& output_file) {
   std::string buffer = ReadOrFail(input_file, "input");
 
   courgette::SourceStreamSet sources;
   if (!sources.Init(buffer.c_str(), buffer.length()))
     Problem("Bad input file.");
 
-  courgette::EncodedProgram* encoded = NULL;
-  const courgette::Status read_status = ReadEncodedProgram(&sources, &encoded);
+  scoped_ptr<courgette::EncodedProgram> encoded;
+  const courgette::Status read_status =
+      courgette::ReadEncodedProgram(&sources, &encoded);
   if (read_status != courgette::C_OK)
     Problem("Bad encoded program.");
 
   courgette::SinkStream sink;
 
-  const courgette::Status assemble_status = courgette::Assemble(encoded, &sink);
+  const courgette::Status assemble_status =
+      courgette::Assemble(encoded.get(), &sink);
   if (assemble_status != courgette::C_OK)
     Problem("Can't assemble.");
 
   WriteSinkToFile(&sink, output_file);
 }
 
 void GenerateEnsemblePatch(const base::FilePath& old_file,
                            const base::FilePath& new_file,
                            const base::FilePath& patch_file) {
   std::string old_buffer = ReadOrFail(old_file, "'old' input");
@@ skipping 194 matching lines @@
         UsageProblem("-gen1[au] <old_file> <new_file> <patch_files_root>");
       DisassembleAdjustDiff(values[0], values[1], values[2],
                             cmd_spread_1_adjusted);
     } else {
       UsageProblem("No operation specified");
     }
   }
 
   return 0;
 }