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

courgette/courgette_tool.cc

Index: courgette/courgette_tool.cc
diff --git a/courgette/courgette_tool.cc b/courgette/courgette_tool.cc
index c7e7196f4c111ab373cb8803230c4b72a55d8223..585a06464d7a59a10ea43ed5804b1d641c1ab218 100644
--- a/courgette/courgette_tool.cc
+++ b/courgette/courgette_tool.cc
@@ -13,10 +13,14 @@
 #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"
 
@@ -76,30 +80,27 @@ 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.");
 
-  courgette::SinkStreamSet sinks;
+  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))
@@ -159,7 +160,7 @@ void DisassembleAndAdjust(const base::FilePath& program_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(),
@@ -167,7 +168,7 @@ void DisassembleAndAdjust(const base::FilePath& program_file,
   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(),
@@ -175,26 +176,26 @@ void DisassembleAndAdjust(const base::FilePath& program_file,
   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);
-
-  courgette::DeleteAssemblyProgram(program);
+  model.reset(nullptr);
 
+  scoped_ptr<courgette::EncodedProgram> encoded;
+  const courgette::Status encode_status = Encode(*program, &encoded);
   if (encode_status != courgette::C_OK)
     Problem("Can't encode program.");
 
-  courgette::SinkStreamSet sinks;
+  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))
@@ -215,7 +216,7 @@ void DisassembleAdjustDiff(const base::FilePath& model_file,
   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(),
@@ -223,7 +224,7 @@ void DisassembleAdjustDiff(const base::FilePath& model_file,
   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(),
@@ -232,37 +233,41 @@ void DisassembleAdjustDiff(const base::FilePath& model_file,
     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) {
@@ -295,14 +300,16 @@ void Assemble(const base::FilePath& input_file,
   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.");